diff --git a/contracts/Safe.sol b/contracts/Safe.sol index e6000223c..2e13aed3a 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 @@ -292,9 +292,13 @@ 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++) { + 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 a74493b40..671909fb1 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; } @@ -60,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); @@ -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); } /** @@ -139,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/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/common/StorageAccessible.sol b/contracts/common/StorageAccessible.sol index 34aa6c612..d4081e96f 100644 --- a/contracts/common/StorageAccessible.sol +++ b/contracts/common/StorageAccessible.sol @@ -17,7 +17,7 @@ abstract contract StorageAccessible { function getStorageAt(uint256 offset, uint256 length) public view returns (bytes memory) { // We use `<< 5` instead of `* 32` as SHR / SHL opcode only uses 3 gas, while DIV / MUL opcode uses 5 gas. bytes memory result = new bytes(length << 5); - 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/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; } } 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 diff --git a/contracts/handler/extensible/ERC165Handler.sol b/contracts/handler/extensible/ERC165Handler.sol index 4833ddcdf..004e1cb18 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); + } } } @@ -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]; 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 { 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 c93efb45f..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 */ @@ -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)); 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; 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"); 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; } 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 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) } }