Merge branch 'main' into g10

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
@@ -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) {

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;
     }
 
@@ -71,13 +72,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);
@@ -110,7 +111,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);
     }
 
     /**

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

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;
     }
 }

contracts/handler/TokenCallbackHandler.sol

@@ -13,15 +13,15 @@ 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;
     }
 
     /**
      * @notice Handles ERC1155 Token batch callback.
-     * return Standardized onERC1155BatchReceived return value.
+     * @return Standardized onERC1155BatchReceived return value.
      */
     function onERC1155BatchReceived(
         address,
@@ -35,15 +35,18 @@ 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;
     }
 
     /**
      * @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

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);
+            }
         }
     }
 
@@ -53,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) {
@@ -75,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];

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 {

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));

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;

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");

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

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)
                 }
             }