From 61b0587731ed7589b2318d991dcbadcf4843a7f9 Mon Sep 17 00:00:00 2001
From: Hanzel Anchia Mena <33629234+hanzel98@users.noreply.github.com>
Date: Wed, 30 Oct 2024 09:43:47 -0600
Subject: [PATCH] October Audit 3.2 Selector and Calldata Length (#463)

---
 src/enforcers/ERC721TransferEnforcer.sol       | 3 ++-
 src/enforcers/OwnershipTransferEnforcer.sol    | 5 +++--
 test/enforcers/OwnershipTransferEnforcer.t.sol | 7 +++++--
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/enforcers/ERC721TransferEnforcer.sol b/src/enforcers/ERC721TransferEnforcer.sol
index ee398b3..825c273 100644
--- 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
index 294ac58..bf1eef2 100644
--- 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
index 358b18e..7d6f8b1 100644
--- 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
         );