From 2fd69b06fc3f4324b652c46618563e9ff02dcd92 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 15:25:35 +0100
Subject: [PATCH 01/15] [Certora Audit] G-02. `OwnerManager.changeThreshold()`
 event update (#889)

This pull request includes a change which allows 1 SLOAD to be saved by
emitting an existing memory variable instead of reading from storage.

*
[`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L113-R113):
Modified the `emit` statement in the `setThreshold` function to use the
`_threshold` parameter instead of `threshold`, ensuring the value from
memory is emitted.
---
 contracts/base/OwnerManager.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/base/OwnerManager.sol b/contracts/base/OwnerManager.sol
index a74493b40..cdb8df63e 100644
--- a/contracts/base/OwnerManager.sol
+++ b/contracts/base/OwnerManager.sol
@@ -110,7 +110,7 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
         // There has to be at least one Safe owner.
         if (_threshold == 0) revertWithError("GS202");
         threshold = _threshold;
-        emit ChangedThreshold(threshold);
+        emit ChangedThreshold(_threshold);
     }
 
     /**

From ff36adb946a500139dd397e780355ff28bf1c8bb Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Thu, 9 Jan 2025 16:03:48 +0100
Subject: [PATCH 02/15] Update comments in SignatureVerifierMuxer to reflect
 correct byte ranges for encodeData and payload lengths (#873)

This pull request includes modifying the `SignatureVerifierMuxer`
contract to correct the byte offsets for encoding and payload data.

Changes to byte offsets:

*
[`contracts/handler/extensible/SignatureVerifierMuxer.sol`](diffhunk://#diff-62f21ce8850527f34ef2acdacd96d4a2a1150e3e2a7e16457e82236bbd4259d2L124-R127):
Updated the byte offset ranges for `encodeData length`, `encodeData`,
and `payload length` to correct values.
---
 .../handler/extensible/SignatureVerifierMuxer.sol  | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/contracts/handler/extensible/SignatureVerifierMuxer.sol b/contracts/handler/extensible/SignatureVerifierMuxer.sol
index c93efb45f..a2b5a0532 100644
--- a/contracts/handler/extensible/SignatureVerifierMuxer.sol
+++ b/contracts/handler/extensible/SignatureVerifierMuxer.sol
@@ -118,13 +118,13 @@ abstract contract SignatureVerifierMuxer is ExtensibleBase, ERC1271, ISignatureV
             if (sigSelector == SAFE_SIGNATURE_MAGIC_VALUE && signature.length >= 68) {
                 // Signature is for an `ISafeSignatureVerifier` - decode the signature.
                 // Layout of the `signature`:
-                // 0x00 - 0x04: selector
-                // 0x04 - 0x36: domainSeparator
-                // 0x36 - 0x68: typeHash
-                // 0x68 - 0x6C: encodeData length
-                // 0x6C - 0x6C + encodeData length: encodeData
-                // 0x6C + encodeData length - 0x6C + encodeData length + 0x20: payload length
-                // 0x6C + encodeData length + 0x20 - end: payload
+                // 0x00 to 0x04: selector
+                // 0x04 to 0x36: domainSeparator
+                // 0x36 to 0x68: typeHash
+                // 0x68 to 0x88: encodeData length
+                // 0x88 to 0x88 + encodeData length: encodeData
+                // 0x88 + encodeData length to 0x88 + encodeData length + 0x20: payload length
+                // 0x88 + encodeData length + 0x20 to end: payload
                 //
                 // Get the domainSeparator from the signature.
                 (bytes32 domainSeparator, bytes32 typeHash) = abi.decode(signature[4:68], (bytes32, bytes32));

From c6cd4b9b0e5ff28ac14246b236679b72566c4c50 Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Thu, 9 Jan 2025 16:37:14 +0100
Subject: [PATCH 03/15] docs: enhance fallback handler documentation in
 `Safe.sol` and `IFallbackManager.sol` (#879)

Updated the documentation for the fallback handler in both `Safe.sol`
and `IFallbackManager.sol` to improve clarity and highlight security
risks associated with setting the fallback handler. Added a warning
about the potential for bypassing access control mechanisms when using
untrusted addresses.
---
 contracts/Safe.sol                        | 2 +-
 contracts/interfaces/IFallbackManager.sol | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/contracts/Safe.sol b/contracts/Safe.sol
index b75eedb50..a05499a33 100644
--- a/contracts/Safe.sol
+++ b/contracts/Safe.sol
@@ -28,7 +28,7 @@ import {Enum} from "./libraries/Enum.sol";
  *          1. Transaction Guard: managed in `GuardManager` for transactions executed with `execTransaction`.
  *          2. Module Guard: managed in `ModuleManager` for transactions executed with `execTransactionFromModule`
  *      - Modules: Modules are contracts that can be used to extend the write functionality of a Safe. Managed in `ModuleManager`.
- *      - Fallback: Fallback handler is a contract that can provide additional read-only functionality for Safe. Managed in `FallbackManager`.
+ *      - Fallback: Fallback handler is a contract that can provide additional functionality for Safe. Managed in `FallbackManager`. Please read the security risks in the `IFallbackManager` interface.
  *      Note: This version of the implementation contract doesn't emit events for the sake of gas efficiency and therefore requires a tracing node for indexing/
  *      For the events-based implementation see `SafeL2.sol`.
  * @author Stefan George - @Georgi87
diff --git a/contracts/interfaces/IFallbackManager.sol b/contracts/interfaces/IFallbackManager.sol
index 45b626d87..5e696465e 100644
--- a/contracts/interfaces/IFallbackManager.sol
+++ b/contracts/interfaces/IFallbackManager.sol
@@ -10,9 +10,12 @@ interface IFallbackManager {
 
     /**
      * @notice Set Fallback Handler to `handler` for the Safe.
-     * @dev Only fallback calls without value and with data will be forwarded.
-     *      This can only be done via a Safe transaction.
-     *      Cannot be set to the Safe itself.
+     * @dev 1. Only fallback calls without value and with data will be forwarded.
+     *      2. Changing the fallback handler can only be done via a Safe transaction.
+     *      3. Cannot be set to the Safe itself.
+     *      4. IMPORTANT! SECURITY RISK! The fallback handler can be set to any address and all the calls will be forwarded to it,
+     *         bypassing all the Safe's access control mechanisms. When setting the fallback handler, make sure to check the address
+     *         is a trusted contract and if it supports state changes, it implements the necessary checks.
      * @param handler contract to handle fallback calls.
      */
     function setFallbackHandler(address handler) external;

From c92ddefe70bc067fc34e549fb939bbc3ffef4ab1 Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Thu, 9 Jan 2025 16:57:21 +0100
Subject: [PATCH 04/15] Enhance documentation in TokenCallbackHandler with
 ERC-1820 registration details (#885)

This PR adds a detailed comment in the TokenCallbackHandler contract,
clarifying the requirement for accounts to register the implementer via
the ERC-1820 interface registry to receive ERC777 tokens. This update
aims to improve clarity and understanding of the token reception
process.

No functional changes were made to the contract logic.
---
 contracts/handler/TokenCallbackHandler.sol | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/contracts/handler/TokenCallbackHandler.sol b/contracts/handler/TokenCallbackHandler.sol
index 4373d2448..4f8bc5557 100644
--- a/contracts/handler/TokenCallbackHandler.sol
+++ b/contracts/handler/TokenCallbackHandler.sol
@@ -13,7 +13,7 @@ import {IERC165} from "../interfaces/IERC165.sol";
 contract TokenCallbackHandler is ERC1155TokenReceiver, ERC777TokensRecipient, ERC721TokenReceiver, IERC165 {
     /**
      * @notice Handles ERC1155 Token callback.
-     * return Standardized onERC1155Received return value.
+     * @return Standardized onERC1155Received return value.
      */
     function onERC1155Received(address, address, uint256, uint256, bytes calldata) external pure override returns (bytes4) {
         return 0xf23a6e61;
@@ -21,7 +21,7 @@ contract TokenCallbackHandler is ERC1155TokenReceiver, ERC777TokensRecipient, ER
 
     /**
      * @notice Handles ERC1155 Token batch callback.
-     * return Standardized onERC1155BatchReceived return value.
+     * @return Standardized onERC1155BatchReceived return value.
      */
     function onERC1155BatchReceived(
         address,
@@ -35,7 +35,7 @@ contract TokenCallbackHandler is ERC1155TokenReceiver, ERC777TokensRecipient, ER
 
     /**
      * @notice Handles ERC721 Token callback.
-     *  return Standardized onERC721Received return value.
+     * @return Standardized onERC721Received return value.
      */
     function onERC721Received(address, address, uint256, bytes calldata) external pure override returns (bytes4) {
         return 0x150b7a02;
@@ -43,7 +43,10 @@ contract TokenCallbackHandler is ERC1155TokenReceiver, ERC777TokensRecipient, ER
 
     /**
      * @notice Handles ERC777 Token callback.
-     * return nothing (not standardized)
+     * @dev Account that wishes to receive the tokens also needs to register the implementer (this contract) via the ERC-1820 interface registry.
+     *      From the standard: "This is done by calling the setInterfaceImplementer function on the ERC-1820 registry with the holder address as
+     *      the address, the keccak256 hash of ERC777TokensSender (0x29ddb589b1fb5fc7cf394961c1adf5f8c6454761adf795e67fe149f658abe895) as the
+     *      interface hash, and the address of the contract implementing the ERC777TokensSender as the implementer."
      */
     function tokensReceived(address, address, address, uint256, bytes calldata, bytes calldata) external pure override {
         // We implement this for completeness, doesn't really have any value

From a1925742072e17ed8b2ed3213fb8caacb17aea84 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 17:08:19 +0100
Subject: [PATCH 05/15] [Certora Audit] I-02. Some comments say `keccak`
 instead of `keccak256` (#886)

This pull request includes updates to comments in the Solidity smart
contracts to clarify the usage of the `keccak256` function. The changes
ensure that the comments accurately reflect the encoding process used in
the contracts.

Changes to comments for clarity:

*
[`contracts/common/SecuredTokenTransfer.sol`](diffhunk://#diff-7a34930a339acfe3b45e163bee3e08df2132c01826e6e03771827a4181c6f567L19-R19):
Updated the comment to specify that `0xa9059cbb` is the `bytes4`
encoding of `keccak256("transfer(address,uint256)")`.
*
[`contracts/proxies/SafeProxy.sol`](diffhunk://#diff-754f853ea53666aa85b2d27bbcc0623b7cfd83449e0b949eed39dde8f01ba1cdL41-R41):
Updated the comment to specify that `0xa619486e` is the `bytes4`
encoding of `keccak256("masterCopy()")`.

Co-authored-by: Mikhail <16622558+mmv08@users.noreply.github.com>
---
 contracts/common/SecuredTokenTransfer.sol | 2 +-
 contracts/proxies/SafeProxy.sol           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contracts/common/SecuredTokenTransfer.sol b/contracts/common/SecuredTokenTransfer.sol
index 359eb231a..382bcb059 100644
--- a/contracts/common/SecuredTokenTransfer.sol
+++ b/contracts/common/SecuredTokenTransfer.sol
@@ -16,7 +16,7 @@ abstract contract SecuredTokenTransfer {
      * @return transferred Returns true if the transfer was successful
      */
     function transferToken(address token, address receiver, uint256 amount) internal returns (bool transferred) {
-        // 0xa9059cbb - keccak("transfer(address,uint256)")
+        // 0xa9059cbb - bytes4(keccak256("transfer(address,uint256)"))
         bytes memory data = abi.encodeWithSelector(0xa9059cbb, receiver, amount);
         /* solhint-disable no-inline-assembly */
         /// @solidity memory-safe-assembly
diff --git a/contracts/proxies/SafeProxy.sol b/contracts/proxies/SafeProxy.sol
index 42e0f477c..10c4931af 100644
--- a/contracts/proxies/SafeProxy.sol
+++ b/contracts/proxies/SafeProxy.sol
@@ -38,7 +38,7 @@ contract SafeProxy {
         /* solhint-disable no-inline-assembly */
         assembly {
             let _singleton := sload(0)
-            // 0xa619486e == keccak("masterCopy()"). The value is right padded to 32-bytes with 0s
+            // 0xa619486e == bytes4(keccak256("masterCopy()")). The value is right padded to 32-bytes with 0s
             if eq(calldataload(0), 0xa619486e00000000000000000000000000000000000000000000000000000000) {
                 // We mask the singleton address when handling the `masterCopy()` call to ensure that it is correctly
                 // ABI-encoded. We do this by shifting the address left by 96 bits (or 12 bytes) and then storing it in

From 183a588fe942d851d697dc5f6d6bd2d0e9654bb5 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 17:19:36 +0100
Subject: [PATCH 06/15] [Certora Audit] G-01. `OwnerManager.removeOwner()`: 1
 SLOAD can be saved in the normal path (#888)

This pull request includes a small but important change to the
`removeOwner` function in the `OwnerManager` contract. The change
optimizes the decrement operation for the `ownerCount` variable.

The previous code reads from storage twice with the `ownerCount`
variable. By doing pre-decrement (which is cheaper than post-decrement),
we can save on 1 SLOAD.

Optimization:

*
[`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L74-L80):
Modified the `removeOwner` function to use pre-decrement for
`ownerCount` to improve efficiency and ensure the threshold check is
performed correctly.
---
 contracts/base/OwnerManager.sol | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contracts/base/OwnerManager.sol b/contracts/base/OwnerManager.sol
index cdb8df63e..725ea8d3d 100644
--- a/contracts/base/OwnerManager.sol
+++ b/contracts/base/OwnerManager.sol
@@ -71,13 +71,13 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
      */
     function removeOwner(address prevOwner, address owner, uint256 _threshold) public override authorized {
         // Only allow to remove an owner, if threshold can still be reached.
-        if (ownerCount - 1 < _threshold) revertWithError("GS201");
+        // Here we do pre-decrement as it is cheaper and allows us to check if the threshold is still reachable.
+        if (--ownerCount < _threshold) revertWithError("GS201");
         // Validate owner address and check that it corresponds to owner index.
         if (owner == address(0) || owner == SENTINEL_OWNERS) revertWithError("GS203");
         if (owners[prevOwner] != owner) revertWithError("GS205");
         owners[prevOwner] = owners[owner];
         owners[owner] = address(0);
-        ownerCount--;
         emit RemovedOwner(owner);
         // Change threshold if threshold was changed.
         if (threshold != _threshold) changeThreshold(_threshold);

From 95f8cb9c906db17384604f6eb978acf07264d11f Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 17:20:32 +0100
Subject: [PATCH 07/15] [Certora Audit] G-03.
 `ERC165Handler.setSupportedInterface()`: Logic and storage access
 optimization (#890)

This pull request includes an important update to the `ERC165Handler`
contract to improve the handling of interface support and streamline the
code. The most significant changes are as follows:

### The following optimizes the logic and minimizes storage accesses:

*
[`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L38-R48):
Refactored the logic for checking and updating the support status of
interfaces by introducing a local `mapping` reference. This change
simplifies the code and ensures that the interface support status is
updated correctly.
---
 contracts/handler/extensible/ERC165Handler.sol | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/contracts/handler/extensible/ERC165Handler.sol b/contracts/handler/extensible/ERC165Handler.sol
index 4833ddcdf..cf3ece3ed 100644
--- a/contracts/handler/extensible/ERC165Handler.sol
+++ b/contracts/handler/extensible/ERC165Handler.sol
@@ -35,13 +35,15 @@ abstract contract ERC165Handler is ExtensibleBase, IERC165Handler {
         ISafe safe = ISafe(payable(_manager()));
         // invalid interface id per ERC165 spec
         require(interfaceId != 0xffffffff, "invalid interface id");
-        bool current = safeInterfaces[safe][interfaceId];
-        if (supported && !current) {
-            safeInterfaces[safe][interfaceId] = true;
-            emit AddedInterface(safe, interfaceId);
-        } else if (!supported && current) {
-            delete safeInterfaces[safe][interfaceId];
-            emit RemovedInterface(safe, interfaceId);
+        mapping(bytes4 => bool) storage safeInterface = safeInterfaces[safe];
+        bool current = safeInterface[interfaceId];
+        if (supported != current) {
+            safeInterface[interfaceId] = supported;
+            if (supported) {
+                emit AddedInterface(safe, interfaceId);
+            } else {
+                emit RemovedInterface(safe, interfaceId);
+            }
         }
     }
 

From 25365fcad1658b608a7cb783ed56a397666157d6 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 17:21:13 +0100
Subject: [PATCH 08/15] [Certora Audit] G-04.
 `ExtensibleBase._setSafeMethod()`: storage access optimization (#891)

This pull request includes a refactor to the `_setSafeMethod` function
in the `ExtensibleBase` contract to improve memory access.

The following optimizes the logic and minimizes storage accesses:

*
[`contracts/handler/extensible/ExtensibleBase.sol`](diffhunk://#diff-e395e05c7e2951461c2e599137367a6a3304e849f3c522903d74f1a50f23b577L49-R57):
Modified the `_setSafeMethod` function to use a local variable
`safeMethod` for accessing the `safeMethods` mapping, which simplifies
and clarifies the code.
---
 contracts/handler/extensible/ExtensibleBase.sol | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/contracts/handler/extensible/ExtensibleBase.sol b/contracts/handler/extensible/ExtensibleBase.sol
index 6c1e19a7c..75c7c924c 100644
--- a/contracts/handler/extensible/ExtensibleBase.sol
+++ b/contracts/handler/extensible/ExtensibleBase.sol
@@ -46,14 +46,15 @@ abstract contract ExtensibleBase is HandlerContext {
 
     function _setSafeMethod(ISafe safe, bytes4 selector, bytes32 newMethod) internal {
         (, address newHandler) = MarshalLib.decode(newMethod);
-        bytes32 oldMethod = safeMethods[safe][selector];
+        mapping(bytes4 => bytes32) storage safeMethod = safeMethods[safe];
+        bytes32 oldMethod = safeMethod[selector];
         (, address oldHandler) = MarshalLib.decode(oldMethod);
 
         if (address(newHandler) == address(0) && address(oldHandler) != address(0)) {
-            delete safeMethods[safe][selector];
+            delete safeMethod[selector];
             emit RemovedSafeMethod(safe, selector);
         } else {
-            safeMethods[safe][selector] = newMethod;
+            safeMethod[selector] = newMethod;
             if (address(oldHandler) == address(0)) {
                 emit AddedSafeMethod(safe, selector, newMethod);
             } else {

From 0e061e24d8d7042c44968158430f0027e4b9df83 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 17:21:56 +0100
Subject: [PATCH 09/15] [Certora Audit] G-05. Use `iszero` instead of `eq(*,
 0)` (#892)

This pull request includes a critical update to the `SafeProxyFactory`
contract in the `contracts/proxies/SafeProxyFactory.sol` file. The
change improves the gas cost of the assembly code by replacing the `eq`
function with the `iszero` function.

Key change:

*
[`contracts/proxies/SafeProxyFactory.sol`](diffhunk://#diff-53aac98a01e16743466b00bdcb233208f95e1113cc90b95181187d9862b4ad6eL43-R43):
Replaced the `eq` function with the `iszero` function in the assembly
block to improve code readability and reliability.
---
 contracts/proxies/SafeProxyFactory.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/proxies/SafeProxyFactory.sol b/contracts/proxies/SafeProxyFactory.sol
index bd470ced9..b8f6f1ce6 100644
--- a/contracts/proxies/SafeProxyFactory.sol
+++ b/contracts/proxies/SafeProxyFactory.sol
@@ -40,7 +40,7 @@ contract SafeProxyFactory {
             /* solhint-disable no-inline-assembly */
             /// @solidity memory-safe-assembly
             assembly {
-                if eq(call(gas(), proxy, 0, add(initializer, 0x20), mload(initializer), 0, 0), 0) {
+                if iszero(call(gas(), proxy, 0, add(initializer, 0x20), mload(initializer), 0, 0)) {
                     revert(0, 0)
                 }
             }

From e35793d5fbbfb884c048f1fc5c5b2d70fecd80d5 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 17:23:01 +0100
Subject: [PATCH 10/15] [Certora Audit] G-06.
 `ExtensibleFallbackHandler._supportsInterface()`: save gas via short-circuit
 evaluation (#893)

This pull request includes a change to the `ExtensibleFallbackHandler`
contract in the `contracts/handler/ExtensibleFallbackHandler.sol` file.
The change modifies the `_supportsInterface` function to reorder the
interface checks for `ERC721TokenReceiver`, `ERC1155TokenReceiver`, and
`IFallbackHandler`. This helps in taking advantage of the short-circuit
evaluation.

Changes to interface support order:

*
[`contracts/handler/ExtensibleFallbackHandler.sol`](diffhunk://#diff-aa345618c4d3f173b09e211d0bd0eec0747177aab345bf8b9f5bbc874a765fe3R21-R26):
Reordered the interface checks in the `_supportsInterface` function to
place `ERC721TokenReceiver` and `ERC1155TokenReceiver` before
`IFallbackHandler`.
---
 contracts/handler/ExtensibleFallbackHandler.sol | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contracts/handler/ExtensibleFallbackHandler.sol b/contracts/handler/ExtensibleFallbackHandler.sol
index 7843af4b2..694474c48 100644
--- a/contracts/handler/ExtensibleFallbackHandler.sol
+++ b/contracts/handler/ExtensibleFallbackHandler.sol
@@ -18,11 +18,11 @@ contract ExtensibleFallbackHandler is FallbackHandler, SignatureVerifierMuxer, T
      */
     function _supportsInterface(bytes4 interfaceId) internal pure override returns (bool) {
         return
+            interfaceId == type(ERC721TokenReceiver).interfaceId ||
+            interfaceId == type(ERC1155TokenReceiver).interfaceId ||
             interfaceId == type(ERC1271).interfaceId ||
             interfaceId == type(ISignatureVerifierMuxer).interfaceId ||
             interfaceId == type(ERC165Handler).interfaceId ||
-            interfaceId == type(IFallbackHandler).interfaceId ||
-            interfaceId == type(ERC721TokenReceiver).interfaceId ||
-            interfaceId == type(ERC1155TokenReceiver).interfaceId;
+            interfaceId == type(IFallbackHandler).interfaceId;
     }
 }

From b2c60870a9d95aa77f75a605b63c22798aaf9938 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 17:48:02 +0100
Subject: [PATCH 11/15] [Certora Audit] G-09. Cache array length outside of
 loop (#896)

This pull request includes several optimizations to the `OwnerManager`
and `ERC165Handler` contracts by reducing the number of times array
lengths are accessed within loops. These changes aim to improve the
efficiency of the code by storing the array lengths in variables before
the loops.

If not cached, the solidity compiler will always read the length of the
array during each iteration. That is, if it is a storage array, this is
an extra sload operation (100 additional extra gas for each iteration
except for the first) and if it is a memory array, this is an extra
mload operation (3 additional gas for each iteration except for the
first).

Optimizations in `OwnerManager` contract:

*
[`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L38-R39):
Stored `_owners.length` in a variable `ownersLength` before the loop to
avoid repeatedly accessing the array length.
*
[`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L49-R50):
Updated `ownerCount` to use the `ownersLength` variable instead of
accessing `_owners.length` again.

Optimizations in `ERC165Handler` contract:

*
[`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L56-R57):
Stored `handlerWithSelectors.length` in a variable `len` before the loop
in `addSupportedInterfaceBatch` to avoid repeatedly accessing the array
length.
*
[`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L78-R80):
Stored `selectors.length` in a variable `len` before the loop in
`removeSupportedInterfaceBatch` to avoid repeatedly accessing the array
length.
---
 contracts/base/OwnerManager.sol                | 5 +++--
 contracts/handler/extensible/ERC165Handler.sol | 6 ++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/contracts/base/OwnerManager.sol b/contracts/base/OwnerManager.sol
index 725ea8d3d..77821523a 100644
--- a/contracts/base/OwnerManager.sol
+++ b/contracts/base/OwnerManager.sol
@@ -35,7 +35,8 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
         if (_threshold == 0) revertWithError("GS202");
         // Initializing Safe owners.
         address currentOwner = SENTINEL_OWNERS;
-        for (uint256 i = 0; i < _owners.length; i++) {
+        uint256 ownersLength = _owners.length;
+        for (uint256 i = 0; i < ownersLength; i++) {
             // Owner address cannot be null.
             address owner = _owners[i];
             if (owner == address(0) || owner == SENTINEL_OWNERS || owner == address(this) || currentOwner == owner)
@@ -46,7 +47,7 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
             currentOwner = owner;
         }
         owners[currentOwner] = SENTINEL_OWNERS;
-        ownerCount = _owners.length;
+        ownerCount = ownersLength;
         threshold = _threshold;
     }
 
diff --git a/contracts/handler/extensible/ERC165Handler.sol b/contracts/handler/extensible/ERC165Handler.sol
index cf3ece3ed..bc19fe624 100644
--- a/contracts/handler/extensible/ERC165Handler.sol
+++ b/contracts/handler/extensible/ERC165Handler.sol
@@ -55,7 +55,8 @@ abstract contract ERC165Handler is ExtensibleBase, IERC165Handler {
     function addSupportedInterfaceBatch(bytes4 _interfaceId, bytes32[] calldata handlerWithSelectors) external override onlySelf {
         ISafe safe = ISafe(payable(_msgSender()));
         bytes4 interfaceId;
-        for (uint256 i = 0; i < handlerWithSelectors.length; i++) {
+        uint256 len = handlerWithSelectors.length;
+        for (uint256 i = 0; i < len; i++) {
             (bool isStatic, bytes4 selector, address handlerAddress) = MarshalLib.decodeWithSelector(handlerWithSelectors[i]);
             _setSafeMethod(safe, selector, MarshalLib.encode(isStatic, handlerAddress));
             if (i > 0) {
@@ -77,7 +78,8 @@ abstract contract ERC165Handler is ExtensibleBase, IERC165Handler {
     function removeSupportedInterfaceBatch(bytes4 _interfaceId, bytes4[] calldata selectors) external override onlySelf {
         ISafe safe = ISafe(payable(_msgSender()));
         bytes4 interfaceId;
-        for (uint256 i = 0; i < selectors.length; i++) {
+        uint256 len = selectors.length;
+        for (uint256 i = 0; i < len; i++) {
             _setSafeMethod(safe, selectors[i], bytes32(0));
             if (i > 0) {
                 interfaceId ^= selectors[i];

From 8aa455162b279c7ba629fa35267bdc9ac41cecff Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Thu, 9 Jan 2025 18:08:57 +0100
Subject: [PATCH 12/15] [Certora Audit] I-03. Inconsistency in formula for
 `performCreate` and `performCreate2` (#887)

Commutativity makes the two additions equivalent but Certora recommend
the fix below for
readability and to follow the standard given that:
- `deploymentData` gives a pointer to the start of the array (length
position).
- Adding `0x20` skips the first 32 bytes (length field) to point
directly to the start of the payload.

Change:
*
[`contracts/libraries/CreateCall.sol`](diffhunk://#diff-d5d801f238d69f94974c4f4628197fcc2df478f608c18c5f691f73dfd552e36eL25-R25):
Changed the order of parameters in the `create2` function call to
correctly add the offset to `deploymentData`.

Co-authored-by: Mikhail <16622558+mmv08@users.noreply.github.com>
---
 contracts/libraries/CreateCall.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/libraries/CreateCall.sol b/contracts/libraries/CreateCall.sol
index 735d6f118..8054c8356 100644
--- a/contracts/libraries/CreateCall.sol
+++ b/contracts/libraries/CreateCall.sol
@@ -22,7 +22,7 @@ contract CreateCall {
         /* solhint-disable no-inline-assembly */
         /// @solidity memory-safe-assembly
         assembly {
-            newContract := create2(value, add(0x20, deploymentData), mload(deploymentData), salt)
+            newContract := create2(value, add(deploymentData, 0x20), mload(deploymentData), salt)
         }
         /* solhint-enable no-inline-assembly */
         require(newContract != address(0), "Could not deploy contract");

From 19e1d63341315c7655a584f01b675b5855f59505 Mon Sep 17 00:00:00 2001
From: Mikhail <16622558+mmv08@users.noreply.github.com>
Date: Thu, 9 Jan 2025 18:23:43 +0100
Subject: [PATCH 13/15] Enhance Safe.sol with ECDSA malleability warning (#877)

Added a comment in the Safe contract to clarify that the `s` value of
ECDSA signatures is not enforced to be in the lower half of the curve.
This note explains the implications of ECDSA malleability and reassures
that existing mechanisms are in place to prevent duplicate signatures
and replay attacks. No functional changes were made to the contract
logic.
---
 contracts/Safe.sol | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contracts/Safe.sol b/contracts/Safe.sol
index a05499a33..975dc836f 100644
--- a/contracts/Safe.sol
+++ b/contracts/Safe.sol
@@ -291,6 +291,10 @@ contract Safe is
         address currentOwner;
         uint256 v; // Implicit conversion from uint8 to uint256 will be done for v received from signatureSplit(...).
         bytes32 r;
+        // NOTE: We do not enforce the `s` to be from the lower half of the curve
+        // This essentially means that for every signature, there's another valid signature (known as ECDSA malleability)
+        // Since we have other mechanisms to prevent duplicated signatures (ordered owners array) and replay protection (nonce),
+        // we can safely ignore this malleability.
         bytes32 s;
         uint256 i;
         for (i = 0; i < requiredSignatures; i++) {

From 8137b6895f7d51f119838a5470ece51fa5a01d66 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Fri, 10 Jan 2025 12:46:37 +0100
Subject: [PATCH 14/15] [Certora Audit] G-10. ++i costs less gas compared to
 i++ or i+=1 (#897)

This pull request includes several changes to increment and decrement
operations in various Solidity contract files. The primary goal is to
decrease gas usage.

Pre-increments and pre-decrements are cheaper.
For a uint256 i variable, the following is true with the Optimizer
enabled at 10k:
Increment:
- i += 1 is the most expensive form
- i++ costs 6 gas less than i += 1
- ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:
- i -= 1 is the most expensive form
- i-- costs 11 gas less than i -= 1
- --i costs 5 gas less than i-- (16 gas less than i -= 1)

Changes to increment and decrement operations:

*
[`contracts/Safe.sol`](diffhunk://#diff-587b494ea631bb6b7adf4fc3e1a2e6a277a385ff16e1163b26e39de24e9483deL296-R296):
Updated the for loop to use the prefix increment operator in the `Safe`
contract.
*
[`contracts/base/ModuleManager.sol`](diffhunk://#diff-82762908b9416ddadffb149ee4d25f328078fc27f938d454d8a207aad1ec3839L215-R215):
Changed the increment operation to use the prefix increment operator in
the `ModuleManager` contract.
*
[`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L38-R38):
Multiple updates to use prefix increment and decrement operators in the
`OwnerManager` contract.
[[1]](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L38-R38)
[[2]](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L63-R63)
[[3]](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L80-R80)
[[4]](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L142-R142)
*
[`contracts/common/StorageAccessible.sol`](diffhunk://#diff-a7dd65d90b0567bb9ba14ecd4ff414529a934cd3752ccf309800fad93fba354eL19-R19):
Modified the for loop to use the prefix increment operator in the
`StorageAccessible` contract.
*
[`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L56-R56):
Updated for loops to use the prefix increment operator in the
`ERC165Handler` contract.
[[1]](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L56-R56)
[[2]](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L78-R78)
*
[`contracts/libraries/SafeToL2Migration.sol`](diffhunk://#diff-925588b812f729cc164d14a48e571ce813e2f0ae6f5c5420fc0382c767287fd0L188-R188):
Changed the increment operation to use the prefix increment operator in
the `SafeToL2Migration` contract.
---
 contracts/Safe.sol                             | 2 +-
 contracts/base/ModuleManager.sol               | 2 +-
 contracts/base/OwnerManager.sol                | 6 +++---
 contracts/common/StorageAccessible.sol         | 2 +-
 contracts/handler/extensible/ERC165Handler.sol | 4 ++--
 contracts/libraries/SafeToL2Migration.sol      | 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/contracts/Safe.sol b/contracts/Safe.sol
index 975dc836f..3632b3dd3 100644
--- a/contracts/Safe.sol
+++ b/contracts/Safe.sol
@@ -297,7 +297,7 @@ contract Safe is
         // we can safely ignore this malleability.
         bytes32 s;
         uint256 i;
-        for (i = 0; i < requiredSignatures; i++) {
+        for (i = 0; i < requiredSignatures; ++i) {
             (v, r, s) = signatureSplit(signatures, i);
             if (v == 0) {
                 // If v is 0 then it is a contract signature
diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol
index b3985be85..9b1c9d409 100644
--- a/contracts/base/ModuleManager.sol
+++ b/contracts/base/ModuleManager.sol
@@ -212,7 +212,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager {
         while (next != address(0) && next != SENTINEL_MODULES && moduleCount < pageSize) {
             array[moduleCount] = next;
             next = modules[next];
-            moduleCount++;
+            ++moduleCount;
         }
 
         /**
diff --git a/contracts/base/OwnerManager.sol b/contracts/base/OwnerManager.sol
index 77821523a..671909fb1 100644
--- a/contracts/base/OwnerManager.sol
+++ b/contracts/base/OwnerManager.sol
@@ -36,7 +36,7 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
         // Initializing Safe owners.
         address currentOwner = SENTINEL_OWNERS;
         uint256 ownersLength = _owners.length;
-        for (uint256 i = 0; i < ownersLength; i++) {
+        for (uint256 i = 0; i < ownersLength; ++i) {
             // Owner address cannot be null.
             address owner = _owners[i];
             if (owner == address(0) || owner == SENTINEL_OWNERS || owner == address(this) || currentOwner == owner)
@@ -61,7 +61,7 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
         if (owners[owner] != address(0)) revertWithError("GS204");
         owners[owner] = owners[SENTINEL_OWNERS];
         owners[SENTINEL_OWNERS] = owner;
-        ownerCount++;
+        ++ownerCount;
         emit AddedOwner(owner);
         // Change threshold if threshold was changed.
         if (threshold != _threshold) changeThreshold(_threshold);
@@ -140,7 +140,7 @@ abstract contract OwnerManager is SelfAuthorized, IOwnerManager {
         while (currentOwner != SENTINEL_OWNERS) {
             array[index] = currentOwner;
             currentOwner = owners[currentOwner];
-            index++;
+            ++index;
         }
         return array;
     }
diff --git a/contracts/common/StorageAccessible.sol b/contracts/common/StorageAccessible.sol
index 9b4e6a9c3..c9862910f 100644
--- a/contracts/common/StorageAccessible.sol
+++ b/contracts/common/StorageAccessible.sol
@@ -16,7 +16,7 @@ abstract contract StorageAccessible {
      */
     function getStorageAt(uint256 offset, uint256 length) public view returns (bytes memory) {
         bytes memory result = new bytes(length * 32);
-        for (uint256 index = 0; index < length; index++) {
+        for (uint256 index = 0; index < length; ++index) {
             /* solhint-disable no-inline-assembly */
             /// @solidity memory-safe-assembly
             assembly {
diff --git a/contracts/handler/extensible/ERC165Handler.sol b/contracts/handler/extensible/ERC165Handler.sol
index bc19fe624..004e1cb18 100644
--- a/contracts/handler/extensible/ERC165Handler.sol
+++ b/contracts/handler/extensible/ERC165Handler.sol
@@ -56,7 +56,7 @@ abstract contract ERC165Handler is ExtensibleBase, IERC165Handler {
         ISafe safe = ISafe(payable(_msgSender()));
         bytes4 interfaceId;
         uint256 len = handlerWithSelectors.length;
-        for (uint256 i = 0; i < len; i++) {
+        for (uint256 i = 0; i < len; ++i) {
             (bool isStatic, bytes4 selector, address handlerAddress) = MarshalLib.decodeWithSelector(handlerWithSelectors[i]);
             _setSafeMethod(safe, selector, MarshalLib.encode(isStatic, handlerAddress));
             if (i > 0) {
@@ -79,7 +79,7 @@ abstract contract ERC165Handler is ExtensibleBase, IERC165Handler {
         ISafe safe = ISafe(payable(_msgSender()));
         bytes4 interfaceId;
         uint256 len = selectors.length;
-        for (uint256 i = 0; i < len; i++) {
+        for (uint256 i = 0; i < len; ++i) {
             _setSafeMethod(safe, selectors[i], bytes32(0));
             if (i > 0) {
                 interfaceId ^= selectors[i];
diff --git a/contracts/libraries/SafeToL2Migration.sol b/contracts/libraries/SafeToL2Migration.sol
index 680b17385..176f2e926 100644
--- a/contracts/libraries/SafeToL2Migration.sol
+++ b/contracts/libraries/SafeToL2Migration.sol
@@ -185,7 +185,7 @@ contract SafeToL2Migration is SafeStorage {
         while (currentOwner != sentinelOwners) {
             array[index] = currentOwner;
             currentOwner = owners[currentOwner];
-            index++;
+            ++index;
         }
         return array;
     }

From 5c8c6c0601206ca12829f7f5337b31b4d64885b2 Mon Sep 17 00:00:00 2001
From: Shebin John <admin@remedcu.com>
Date: Fri, 10 Jan 2025 12:46:56 +0100
Subject: [PATCH 15/15] [Certora Audit] G07. Use a mask instead of shifting
 left and right (#894)

This pull request includes changes to the `contracts/handler/extensible`
directory, specifically in the `MarshalLib.sol` and
`SignatureVerifierMuxer.sol` files. The changes focus on improving the
handling of data and selectors within assembly code blocks to decrease
gas usage.

Improvements to data handling and selector extraction:

*
[`contracts/handler/extensible/MarshalLib.sol`](diffhunk://#diff-7122c44132b6fc89cd7c9f3c48519c88aaf7308705a1170d307d72465eb9e1c9L41-R41):
Modified the way the `handler` is extracted from `data` by using a
bitwise AND operation to ensure proper extraction of the handler
address.
[[1]](diffhunk://#diff-7122c44132b6fc89cd7c9f3c48519c88aaf7308705a1170d307d72465eb9e1c9L41-R41)
[[2]](diffhunk://#diff-7122c44132b6fc89cd7c9f3c48519c88aaf7308705a1170d307d72465eb9e1c9L59-R59)
*
[`contracts/handler/extensible/SignatureVerifierMuxer.sol`](diffhunk://#diff-62f21ce8850527f34ef2acdacd96d4a2a1150e3e2a7e16457e82236bbd4259d2L113-R113):
Changed the extraction of `sigSelector` from `calldataload` to use a
bitwise AND operation for more accurate and secure selector extraction.
---
 contracts/handler/extensible/MarshalLib.sol             | 4 ++--
 contracts/handler/extensible/SignatureVerifierMuxer.sol | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/contracts/handler/extensible/MarshalLib.sol b/contracts/handler/extensible/MarshalLib.sol
index b55436e7a..82b041394 100644
--- a/contracts/handler/extensible/MarshalLib.sol
+++ b/contracts/handler/extensible/MarshalLib.sol
@@ -38,7 +38,7 @@ library MarshalLib {
         assembly {
             // set isStatic to true if the left-most byte of the data is 0x00
             isStatic := iszero(shr(248, data))
-            handler := shr(96, shl(96, data))
+            handler := and(data, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
         }
         /* solhint-enable no-inline-assembly */
     }
@@ -56,7 +56,7 @@ library MarshalLib {
         assembly {
             // set isStatic to true if the left-most byte of the data is 0x00
             isStatic := iszero(shr(248, data))
-            handler := shr(96, shl(96, data))
+            handler := and(data, 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF)
             selector := shl(168, shr(160, data))
         }
         /* solhint-enable no-inline-assembly */
diff --git a/contracts/handler/extensible/SignatureVerifierMuxer.sol b/contracts/handler/extensible/SignatureVerifierMuxer.sol
index a2b5a0532..a1ee509f3 100644
--- a/contracts/handler/extensible/SignatureVerifierMuxer.sol
+++ b/contracts/handler/extensible/SignatureVerifierMuxer.sol
@@ -110,7 +110,7 @@ abstract contract SignatureVerifierMuxer is ExtensibleBase, ERC1271, ISignatureV
             /* solhint-disable no-inline-assembly */
             /// @solidity memory-safe-assembly
             assembly {
-                sigSelector := shl(224, shr(224, calldataload(signature.offset)))
+                sigSelector := calldataload(signature.offset)
             }
             /* solhint-enable no-inline-assembly */