October Audit 3.2 Selector and Calldata Length (#463)

MetaMask · Oct 30, 2024 · 61b0587 · 61b0587
1 parent 87c50b1
commit 61b0587
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 5 deletions.
diff --git a/src/enforcers/ERC721TransferEnforcer.sol b/src/enforcers/ERC721TransferEnforcer.sol
@@ -33,7 +33,6 @@ contract ERC721TransferEnforcer is CaveatEnforcer {
     {
         (address permittedContract_, uint256 permittedTokenId_) = getTermsInfo(_terms);
         (address target_,, bytes calldata callData_) = ExecutionLib.decodeSingle(_executionCallData);
-        bytes4 selector_ = bytes4(callData_[0:4]);
 
         // Decode the remaining callData into NFT transfer parameters
         // The calldata should be at least 100 bytes (4 bytes for the selector + 96 bytes for the parameters)
@@ -48,6 +47,8 @@ contract ERC721TransferEnforcer is CaveatEnforcer {
             revert("ERC721TransferEnforcer:invalid-address");
         }
 
+        bytes4 selector_ = bytes4(callData_[0:4]);
+
         if (target_ != permittedContract_) {
             revert("ERC721TransferEnforcer:unauthorized-contract-target");
         } else if (selector_ != IERC721.transferFrom.selector) {

diff --git a/src/enforcers/OwnershipTransferEnforcer.sol b/src/enforcers/OwnershipTransferEnforcer.sol
@@ -72,11 +72,12 @@ contract OwnershipTransferEnforcer is CaveatEnforcer {
         returns (address newOwner_)
     {
         (address target_,, bytes calldata callData_) = _executionCallData.decodeSingle();
-        bytes4 selector = bytes4(callData_[0:4]);
-        require(selector == IERC173.transferOwnership.selector, "OwnershipTransferEnforcer:invalid-method");
 
         require(callData_.length == 36, "OwnershipTransferEnforcer:invalid-execution-length");
 
+        bytes4 selector_ = bytes4(callData_[0:4]);
+        require(selector_ == IERC173.transferOwnership.selector, "OwnershipTransferEnforcer:invalid-method");
+
         address targetContract_ = getTermsInfo(_terms);
         require(targetContract_ == target_, "OwnershipTransferEnforcer:invalid-contract");
 

diff --git a/test/enforcers/OwnershipTransferEnforcer.t.sol b/test/enforcers/OwnershipTransferEnforcer.t.sol
@@ -90,8 +90,11 @@ contract OwnershipTransferEnforcerTest is CaveatEnforcerBaseTest {
     // Reverts if the method called is not transferOwnership
     function test_notAllow_invalidMethod() public {
         bytes memory terms_ = abi.encodePacked(mockContract);
-        transferOwnershipExecution =
-            Execution({ target: mockContract, value: 0, callData: abi.encodeWithSelector(bytes4(keccak256("someOtherMethod()"))) });
+        transferOwnershipExecution = Execution({
+            target: mockContract,
+            value: 0,
+            callData: abi.encodeWithSelector(bytes4(keccak256("someOtherMethod(address)")), address(0))
+        });
         transferOwnershipExecutionCallData = ExecutionLib.encodeSingle(
             transferOwnershipExecution.target, transferOwnershipExecution.value, transferOwnershipExecution.callData
         );