From b1b3137d0282da51c4c91d4224bd00e5e15b978d Mon Sep 17 00:00:00 2001 From: Jan-Felix Date: Thu, 7 Dec 2023 09:27:34 +0100 Subject: [PATCH 1/4] avoid unnecessary external calls --- contracts/ERC1155ThresholdMech.sol | 2 +- contracts/ERC1155TokenboundMech.sol | 7 +++---- contracts/ERC20ThresholdMech.sol | 2 +- contracts/ERC721TokenboundMech.sol | 5 ++--- contracts/base/TokenboundMech.sol | 2 +- 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/contracts/ERC1155ThresholdMech.sol b/contracts/ERC1155ThresholdMech.sol index daff205..468b3ab 100644 --- a/contracts/ERC1155ThresholdMech.sol +++ b/contracts/ERC1155ThresholdMech.sol @@ -32,7 +32,7 @@ contract ERC1155ThresholdMech is ThresholdMech { uint256[] memory tokenIds, uint256[] memory minBalances, uint256 minTotalBalance - ) = this.threshold(); + ) = threshold(); uint256 balanceSum = 0; for (uint256 i = 0; i < tokenIds.length; i++) { diff --git a/contracts/ERC1155TokenboundMech.sol b/contracts/ERC1155TokenboundMech.sol index e76cda1..bace883 100644 --- a/contracts/ERC1155TokenboundMech.sol +++ b/contracts/ERC1155TokenboundMech.sol @@ -9,8 +9,7 @@ import "./base/TokenboundMech.sol"; */ contract ERC1155TokenboundMech is TokenboundMech { function isOperator(address signer) public view override returns (bool) { - (uint256 chainId, address tokenContract, uint256 tokenId) = this - .token(); + (uint256 chainId, address tokenContract, uint256 tokenId) = token(); if (chainId != block.chainid) return false; return IERC1155(tokenContract).balanceOf(signer, tokenId) > 0; } @@ -26,7 +25,7 @@ contract ERC1155TokenboundMech is TokenboundMech { uint256 chainId, address boundTokenContract, uint256 boundTokenId - ) = this.token(); + ) = token(); if ( chainId == block.chainid && @@ -56,7 +55,7 @@ contract ERC1155TokenboundMech is TokenboundMech { uint256 chainId, address boundTokenContract, uint256 boundTokenId - ) = this.token(); + ) = token(); if (chainId == block.chainid && msg.sender == boundTokenContract) { // We block the transfer only if the sender has no balance left after the transfer. diff --git a/contracts/ERC20ThresholdMech.sol b/contracts/ERC20ThresholdMech.sol index 4e91150..fcb39ab 100644 --- a/contracts/ERC20ThresholdMech.sol +++ b/contracts/ERC20ThresholdMech.sol @@ -22,7 +22,7 @@ contract ERC20ThresholdMech is ThresholdMech { } function isOperator(address signer) public view override returns (bool) { - (address token, uint256 minBalance) = this.threshold(); + (address token, uint256 minBalance) = threshold(); return IERC20(token).balanceOf(signer) >= minBalance; } diff --git a/contracts/ERC721TokenboundMech.sol b/contracts/ERC721TokenboundMech.sol index f9f9f86..36be550 100644 --- a/contracts/ERC721TokenboundMech.sol +++ b/contracts/ERC721TokenboundMech.sol @@ -10,8 +10,7 @@ import "./base/TokenboundMech.sol"; */ contract ERC721TokenboundMech is TokenboundMech { function isOperator(address signer) public view override returns (bool) { - (uint256 chainId, address tokenContract, uint256 tokenId) = this - .token(); + (uint256 chainId, address tokenContract, uint256 tokenId) = token(); if (chainId != block.chainid) return false; return IERC721(tokenContract).ownerOf(tokenId) == signer && @@ -28,7 +27,7 @@ contract ERC721TokenboundMech is TokenboundMech { uint256 chainId, address boundTokenContract, uint256 boundTokenId - ) = this.token(); + ) = token(); if ( chainId == block.chainid && diff --git a/contracts/base/TokenboundMech.sol b/contracts/base/TokenboundMech.sol index d57a1a4..7c409b7 100644 --- a/contracts/base/TokenboundMech.sol +++ b/contracts/base/TokenboundMech.sol @@ -18,7 +18,7 @@ abstract contract TokenboundMech is Mech, IERC6551Account { } function token() - external + public view returns (uint256 chainId, address tokenContract, uint256 tokenId) { From f9f0f689106fa71a574b66ad6cb32e4793b52b53 Mon Sep 17 00:00:00 2001 From: Jan-Felix Date: Wed, 13 Dec 2023 16:17:08 +0100 Subject: [PATCH 2/4] fix copy pasted comments --- contracts/ZodiacMech.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/ZodiacMech.sol b/contracts/ZodiacMech.sol index 21429aa..0816e37 100644 --- a/contracts/ZodiacMech.sol +++ b/contracts/ZodiacMech.sol @@ -50,7 +50,7 @@ contract ZodiacMech is SafeStorage, Mech, IAvatar { return isModuleEnabled(signer); } - /// @dev Passes a transaction to the avatar. + /// @dev Passes a transaction to the mech. /// @notice Can only be called by enabled modules. /// @param to Destination address of module transaction. /// @param value Ether value of mo dule transaction. @@ -65,7 +65,7 @@ contract ZodiacMech is SafeStorage, Mech, IAvatar { (success, ) = _exec(to, value, data, uint8(operation), gasleft()); } - /// @dev Passes a transaction to the avatar, expects return data. + /// @dev Passes a transaction to the mech, expects return data. /// @notice Can only be called by enabled modules. /// @param to Destination address of module transaction. /// @param value Ether value of module transaction. @@ -80,7 +80,7 @@ contract ZodiacMech is SafeStorage, Mech, IAvatar { return _exec(to, value, data, uint8(operation), gasleft()); } - /// @dev Disables a module on the modifier. + /// @dev Disables a module on the mech. /// @notice This can only be called by the owner. /// @param prevModule Module that pointed to the module to be removed in the linked list. /// @param module Module to be removed. From cf79eea8d0f3f8f92b7902748ce0afbc16670d2a Mon Sep 17 00:00:00 2001 From: Jan-Felix Date: Wed, 13 Dec 2023 22:44:22 +0100 Subject: [PATCH 3/4] remove chainId checks we have no cross-chain capabilities anyways. it would lead to bricked mechs in case of network forks --- contracts/ERC1155TokenboundMech.sol | 21 +++++---------------- contracts/ERC721TokenboundMech.sol | 13 +++---------- 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/contracts/ERC1155TokenboundMech.sol b/contracts/ERC1155TokenboundMech.sol index bace883..dbf0d05 100644 --- a/contracts/ERC1155TokenboundMech.sol +++ b/contracts/ERC1155TokenboundMech.sol @@ -9,8 +9,7 @@ import "./base/TokenboundMech.sol"; */ contract ERC1155TokenboundMech is TokenboundMech { function isOperator(address signer) public view override returns (bool) { - (uint256 chainId, address tokenContract, uint256 tokenId) = token(); - if (chainId != block.chainid) return false; + (, address tokenContract, uint256 tokenId) = token(); return IERC1155(tokenContract).balanceOf(signer, tokenId) > 0; } @@ -21,16 +20,10 @@ contract ERC1155TokenboundMech is TokenboundMech { uint256, bytes calldata ) external view override returns (bytes4) { - ( - uint256 chainId, - address boundTokenContract, - uint256 boundTokenId - ) = token(); + (, address boundTokenContract, uint256 boundTokenId) = token(); if ( - chainId == block.chainid && - msg.sender == boundTokenContract && - receivedTokenId == boundTokenId + msg.sender == boundTokenContract && receivedTokenId == boundTokenId ) { // We block the transfer only if the sender has no balance left after the transfer. // Note: ERC-1155 prescribes that balances are updated BEFORE the call to onERC1155Received. @@ -51,13 +44,9 @@ contract ERC1155TokenboundMech is TokenboundMech { uint256[] calldata, bytes calldata ) external view override returns (bytes4) { - ( - uint256 chainId, - address boundTokenContract, - uint256 boundTokenId - ) = token(); + (, address boundTokenContract, uint256 boundTokenId) = token(); - if (chainId == block.chainid && msg.sender == boundTokenContract) { + if (msg.sender == boundTokenContract) { // We block the transfer only if the sender has no balance left after the transfer. // Note: ERC-1155 prescribes that balances are updated BEFORE the call to onERC1155BatchReceived. for (uint256 i = 0; i < ids.length; i++) { diff --git a/contracts/ERC721TokenboundMech.sol b/contracts/ERC721TokenboundMech.sol index 36be550..df29edf 100644 --- a/contracts/ERC721TokenboundMech.sol +++ b/contracts/ERC721TokenboundMech.sol @@ -10,8 +10,7 @@ import "./base/TokenboundMech.sol"; */ contract ERC721TokenboundMech is TokenboundMech { function isOperator(address signer) public view override returns (bool) { - (uint256 chainId, address tokenContract, uint256 tokenId) = token(); - if (chainId != block.chainid) return false; + (, address tokenContract, uint256 tokenId) = token(); return IERC721(tokenContract).ownerOf(tokenId) == signer && signer != address(0); @@ -23,16 +22,10 @@ contract ERC721TokenboundMech is TokenboundMech { uint256 receivedTokenId, bytes calldata ) external view override returns (bytes4) { - ( - uint256 chainId, - address boundTokenContract, - uint256 boundTokenId - ) = token(); + (, address boundTokenContract, uint256 boundTokenId) = token(); if ( - chainId == block.chainid && - msg.sender == boundTokenContract && - receivedTokenId == boundTokenId + msg.sender == boundTokenContract && receivedTokenId == boundTokenId ) { revert OwnershipCycle(); } From 7c401a5464e972d7209385b839129c2a02ce791c Mon Sep 17 00:00:00 2001 From: Jan-Felix Date: Wed, 13 Dec 2023 22:59:44 +0100 Subject: [PATCH 4/4] add security hint about `getModulesPaginated` --- README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/README.md b/README.md index 571058e..3ed79e4 100644 --- a/README.md +++ b/README.md @@ -142,3 +142,12 @@ The same mechanism is implemented by the 6551 account registry. The ZodiacMech uses the same storage layout at the Safe contracts, meaning that an existing Safe instance can be migrated to the ZodiacMech implementation. For migrating a Safe it needs to delegate call the [SafeMigration.sol](contracts/libraries/SafeMigration.sol) contract's `migrate()` function. This will revoke access for the Safe owners so that the account will only be controlled by enabled modules going forwards. + +#### Security hint on `getModulesPaginated()` + +\*\*Attention:\*\* You must never trust the result of `getModulesPaginated()` without extra validation. +Modules can add other modules without these appearing in the list returned by `getModulesPaginated` by writing directly to storage slots via delegate calls. + +This caveat is [known](https://blog.openzeppelin.com/backdooring-gnosis-safe-multisig-wallets) for Safe and also extends to Zodiac Modifiers and Avatars, like ZodiacMech. + +For validating that the return value of `getModulesPaginated` indeed includes the full set of enabled modules it is required to calculate the ZodiacMech contract's storage hash and compare it with the value retrieved via [eth_getProof](https://docs.infura.io/networks/ethereum/json-rpc-methods/eth_getproof).