audit feedback

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

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

contracts/ERC1155TokenboundMech.sol

@@ -9,9 +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();
-        if (chainId != block.chainid) return false;
+        (, address tokenContract, uint256 tokenId) = token();
         return IERC1155(tokenContract).balanceOf(signer, tokenId) > 0;
     }
 
@@ -22,16 +20,10 @@ contract ERC1155TokenboundMech is TokenboundMech {
         uint256,
         bytes calldata
     ) external view override returns (bytes4) {
-        (
-            uint256 chainId,
-            address boundTokenContract,
-            uint256 boundTokenId
-        ) = this.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.
@@ -52,13 +44,9 @@ contract ERC1155TokenboundMech is TokenboundMech {
         uint256[] calldata,
         bytes calldata
     ) external view override returns (bytes4) {
-        (
-            uint256 chainId,
-            address boundTokenContract,
-            uint256 boundTokenId
-        ) = this.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++) {

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

contracts/ERC721TokenboundMech.sol

@@ -10,9 +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();
-        if (chainId != block.chainid) return false;
+        (, address tokenContract, uint256 tokenId) = token();
         return
             IERC721(tokenContract).ownerOf(tokenId) == signer &&
             signer != address(0);
@@ -24,16 +22,10 @@ contract ERC721TokenboundMech is TokenboundMech {
         uint256 receivedTokenId,
         bytes calldata
     ) external view override returns (bytes4) {
-        (
-            uint256 chainId,
-            address boundTokenContract,
-            uint256 boundTokenId
-        ) = this.token();
+        (, address boundTokenContract, uint256 boundTokenId) = token();
 
         if (
-            chainId == block.chainid &&
-            msg.sender == boundTokenContract &&
-            receivedTokenId == boundTokenId
+            msg.sender == boundTokenContract && receivedTokenId == boundTokenId
         ) {
             revert OwnershipCycle();
         }

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.

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