Skip to content

Audit - Feb 2025 - 7.3.3 Support 3 transferFrom function selectors #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hanzel98 merged 1 commit into from
Mar 7, 2025
Merged

Audit - Feb 2025 - 7.3.3 Support 3 transferFrom function selectors #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/enforcers/ERC721TransferEnforcer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ import { ExecutionLib } from "@erc7579/lib/ExecutionLib.sol";
* @notice This enforcer restricts the action of a UserOp to the transfer of a specific ERC721 token.
*/
contract ERC721TransferEnforcer is CaveatEnforcer {
bytes4 private constant SAFE_TRANSFER_FROM_SELECTOR_1 = bytes4(keccak256("safeTransferFrom(address,address,uint256)"));
bytes4 private constant SAFE_TRANSFER_FROM_SELECTOR_2 = bytes4(keccak256("safeTransferFrom(address,address,uint256,bytes)"));

/**
* @notice Enforces that the contract and tokenId are permitted for transfer
* @param _terms abi encoded address of the contract and uint256 of the tokenId
Expand Down Expand Up @@ -51,9 +54,16 @@ contract ERC721TransferEnforcer is CaveatEnforcer {

if (target_ != permittedContract_) {
revert("ERC721TransferEnforcer:unauthorized-contract-target");
} else if (selector_ != IERC721.transferFrom.selector) {
}

if (
selector_ != IERC721.transferFrom.selector && selector_ != SAFE_TRANSFER_FROM_SELECTOR_1
&& selector_ != SAFE_TRANSFER_FROM_SELECTOR_2
) {
revert("ERC721TransferEnforcer:unauthorized-selector");
} else if (transferTokenId_ != permittedTokenId_) {
}

if (transferTokenId_ != permittedTokenId_) {
revert("ERC721TransferEnforcer:unauthorized-token-id");
}
}
Expand Down
174 changes: 151 additions & 23 deletions test/enforcers/ERC721TransferEnforcer.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,12 @@ pragma solidity 0.8.23;

import "forge-std/Test.sol";
import { ModeLib } from "@erc7579/lib/ModeLib.sol";
import { BasicCF721 } from "../utils/BasicCF721.t.sol";
import { ExecutionLib } from "@erc7579/lib/ExecutionLib.sol";

import { Execution, Caveat, Delegation, ModeCode } from "../../src/utils/Types.sol";
import { CaveatEnforcerBaseTest } from "./CaveatEnforcerBaseTest.t.sol";
import { ERC721TransferEnforcer } from "../../src/enforcers/ERC721TransferEnforcer.sol";
import { IDelegationManager } from "../../src/interfaces/IDelegationManager.sol";
import { EncoderLib } from "../../src/libraries/EncoderLib.sol";
import { ICaveatEnforcer } from "../../src/interfaces/ICaveatEnforcer.sol";
import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol";

Expand All @@ -20,41 +19,81 @@ contract ERC721TransferEnforcerTest is CaveatEnforcerBaseTest {

ERC721TransferEnforcer public erc721TransferEnforcer;
ModeCode public mode = ModeLib.encodeSimpleSingle();
IERC721 public mockNFT;
address public constant NFT_CONTRACT = address(0x1234567890123456789012345678901234567890);
uint256 public constant TOKEN_ID = 42;
uint256 public constant TOKEN_ID = 0;
BasicCF721 public token;

////////////////////// Set up //////////////////////

function setUp() public override {
super.setUp();
erc721TransferEnforcer = new ERC721TransferEnforcer();
vm.label(address(erc721TransferEnforcer), "ERC721 Transfer Enforcer");
mockNFT = IERC721(NFT_CONTRACT);

token = new BasicCF721(address(users.alice.deleGator), "ERC721Token", "ERC721Token", "");
vm.label(address(token), "ERC721 Test Token");

vm.prank(address(users.alice.deleGator));
token.selfMint();
}

////////////////////// Valid cases //////////////////////

/// @notice Tests that a valid transfer using transferFrom selector passes.
function test_validTransfer() public {
Execution memory execution_ = Execution({
target: NFT_CONTRACT,
target: address(token),
value: 0,
callData: abi.encodeWithSelector(IERC721.transferFrom.selector, address(this), address(0xBEEF), TOKEN_ID)
});
bytes memory executionCallData_ = ExecutionLib.encodeSingle(execution_.target, execution_.value, execution_.callData);

vm.prank(address(delegationManager));
erc721TransferEnforcer.beforeHook(
abi.encodePacked(NFT_CONTRACT, TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
abi.encodePacked(address(token), TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
);
}

/// @notice Tests that a valid transfer using safeTransferFrom (3 args) selector passes.
function test_validSafeTransferFrom_3args() public {
Execution memory execution_ = Execution({
target: address(token),
value: 0,
callData: abi.encodeWithSignature("safeTransferFrom(address,address,uint256)", address(this), address(0xBEEF), TOKEN_ID)
});
bytes memory executionCallData_ = ExecutionLib.encodeSingle(execution_.target, execution_.value, execution_.callData);

vm.prank(address(delegationManager));
erc721TransferEnforcer.beforeHook(
abi.encodePacked(address(token), TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
);
}

/// @notice Tests that a valid transfer using safeTransferFrom (4 args) selector passes.
function test_validSafeTransferFrom_4args() public {
Execution memory execution_ = Execution({
target: address(token),
value: 0,
callData: abi.encodeWithSignature(
"safeTransferFrom(address,address,uint256,bytes)", address(this), address(0xBEEF), TOKEN_ID, hex""
)
});
bytes memory executionCallData_ = ExecutionLib.encodeSingle(execution_.target, execution_.value, execution_.callData);

vm.prank(address(delegationManager));
erc721TransferEnforcer.beforeHook(
abi.encodePacked(address(token), TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
);
}

////////////////////// Invalid cases //////////////////////

/// @notice Tests that getTermsInfo reverts if the terms length is invalid.
function test_invalidTermsLength() public {
vm.expectRevert("ERC721TransferEnforcer:invalid-terms-length");
erc721TransferEnforcer.getTermsInfo(abi.encodePacked(NFT_CONTRACT));
erc721TransferEnforcer.getTermsInfo(abi.encodePacked(address(token)));
}

/// @notice Tests that a transfer reverts if the target contract is unauthorized.
function test_unauthorizedTransfer_wrongContract() public {
Execution memory execution_ = Execution({
target: address(0xDEAD),
Expand All @@ -66,28 +105,28 @@ contract ERC721TransferEnforcerTest is CaveatEnforcerBaseTest {
vm.prank(address(delegationManager));
vm.expectRevert("ERC721TransferEnforcer:unauthorized-contract-target");
erc721TransferEnforcer.beforeHook(
abi.encodePacked(NFT_CONTRACT, TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
abi.encodePacked(address(token), TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
);
}

/// @notice Tests that a transfer reverts when using an unauthorized function selector.
function test_unauthorizedSelector_wrongMethod() public {
Execution memory execution_ = Execution({
target: NFT_CONTRACT,
value: 0,
callData: abi.encodeWithSignature("safeTransferFrom(address,address,uint256)", address(this), address(0xBEEF), TOKEN_ID)
});
bytes4 dummySelector_ = bytes4(keccak256("foo(address,address,uint256)"));
bytes memory callData_ = abi.encodeWithSelector(dummySelector_, address(this), address(0xBEEF), TOKEN_ID);
Execution memory execution_ = Execution({ target: address(token), value: 0, callData: callData_ });
bytes memory executionCallData_ = ExecutionLib.encodeSingle(execution_.target, execution_.value, execution_.callData);

vm.prank(address(delegationManager));
vm.expectRevert("ERC721TransferEnforcer:unauthorized-selector");
erc721TransferEnforcer.beforeHook(
abi.encodePacked(NFT_CONTRACT, TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
abi.encodePacked(address(token), TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
);
}

/// @notice Tests that a transfer reverts if the tokenId does not match the permitted token.
function test_unauthorizedTransfer_wrongTokenId() public {
Execution memory execution_ = Execution({
target: NFT_CONTRACT,
target: address(token),
value: 0,
callData: abi.encodeWithSelector(IERC721.transferFrom.selector, address(this), address(0xBEEF), TOKEN_ID + 1)
});
Expand All @@ -96,13 +135,14 @@ contract ERC721TransferEnforcerTest is CaveatEnforcerBaseTest {
vm.prank(address(delegationManager));
vm.expectRevert("ERC721TransferEnforcer:unauthorized-token-id");
erc721TransferEnforcer.beforeHook(
abi.encodePacked(NFT_CONTRACT, TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
abi.encodePacked(address(token), TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
);
}

function test_unauthorizedTransfer_wrongSelector() public {
/// @notice Tests that a transfer reverts if the calldata length is insufficient.
function test_unauthorizedTransfer_wrongSelectorLength() public {
Execution memory execution_ = Execution({
target: NFT_CONTRACT,
target: address(token),
value: 0,
callData: abi.encodeWithSelector(IERC721.approve.selector, address(0xBEEF), TOKEN_ID)
});
Expand All @@ -111,15 +151,21 @@ contract ERC721TransferEnforcerTest is CaveatEnforcerBaseTest {
vm.prank(address(delegationManager));
vm.expectRevert("ERC721TransferEnforcer:invalid-calldata-length");
erc721TransferEnforcer.beforeHook(
abi.encodePacked(NFT_CONTRACT, TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
abi.encodePacked(address(token), TOKEN_ID), hex"", mode, executionCallData_, keccak256(""), address(0), address(0)
);
}

////////////////////// Integration //////////////////////

/// @notice Integration test for valid transfer using transferFrom selector.
function test_validTransferIntegration() public {
// Pre-transfer: ensure the token is initially owned by Alice and record Bob's balance.
uint256 initialRecipientBalance_ = token.balanceOf(address(users.bob.deleGator));
address initialOwner_ = token.ownerOf(TOKEN_ID);
assertEq(initialOwner_, address(users.alice.deleGator), "Initial owner should be Alice");

Execution memory execution_ = Execution({
target: NFT_CONTRACT,
target: address(token),
value: 0,
callData: abi.encodeWithSelector(
IERC721.transferFrom.selector, address(users.alice.deleGator), address(users.bob.deleGator), TOKEN_ID
Expand All @@ -128,7 +174,7 @@ contract ERC721TransferEnforcerTest is CaveatEnforcerBaseTest {

Caveat[] memory caveats_ = new Caveat[](1);
caveats_[0] =
Caveat({ args: hex"", enforcer: address(erc721TransferEnforcer), terms: abi.encodePacked(NFT_CONTRACT, TOKEN_ID) });
Caveat({ args: hex"", enforcer: address(erc721TransferEnforcer), terms: abi.encodePacked(address(token), TOKEN_ID) });
Delegation memory delegation_ = Delegation({
delegate: address(users.bob.deleGator),
delegator: address(users.alice.deleGator),
Expand All @@ -145,6 +191,88 @@ contract ERC721TransferEnforcerTest is CaveatEnforcerBaseTest {

// Execute Bob's UserOp
invokeDelegation_UserOp(users.bob, delegations_, execution_);

// Post-transfer: verify that Bob's balance increased and that the token owner is now Bob.
uint256 finalRecipientBalance_ = token.balanceOf(address(users.bob.deleGator));
address finalOwner_ = token.ownerOf(TOKEN_ID);
assertEq(finalRecipientBalance_, initialRecipientBalance_ + 1, "Recipient balance should increase by 1");
assertEq(finalOwner_, address(users.bob.deleGator), "Token owner should be Bob");
}

/// @notice Integration test for valid transfer using safeTransferFrom (3 args) selector.
function test_validSafeTransferFrom3argsIntegration() public {
uint256 initialRecipientBalance_ = token.balanceOf(address(users.bob.deleGator));
address initialOwner_ = token.ownerOf(TOKEN_ID);
assertEq(initialOwner_, address(users.alice.deleGator), "Initial owner should be Alice");

Execution memory execution_ = Execution({
target: address(token),
value: 0,
callData: abi.encodeWithSignature(
"safeTransferFrom(address,address,uint256)", address(users.alice.deleGator), address(users.bob.deleGator), TOKEN_ID
)
});
Caveat[] memory caveats_ = new Caveat[](1);
caveats_[0] =
Caveat({ args: hex"", enforcer: address(erc721TransferEnforcer), terms: abi.encodePacked(address(token), TOKEN_ID) });
Delegation memory delegation_ = Delegation({
delegate: address(users.bob.deleGator),
delegator: address(users.alice.deleGator),
authority: ROOT_AUTHORITY,
caveats: caveats_,
salt: 0,
signature: hex""
});
delegation_ = signDelegation(users.alice, delegation_);
Delegation[] memory delegations_ = new Delegation[](1);
delegations_[0] = delegation_;
// Execute Bob's UserOp with safeTransferFrom (3 args)
invokeDelegation_UserOp(users.bob, delegations_, execution_);

uint256 finalRecipientBalance_ = token.balanceOf(address(users.bob.deleGator));
address finalOwner_ = token.ownerOf(TOKEN_ID);
assertEq(finalRecipientBalance_, initialRecipientBalance_ + 1, "Recipient balance should increase by 1");
assertEq(finalOwner_, address(users.bob.deleGator), "Token owner should be Bob");
}

/// @notice Integration test for valid transfer using safeTransferFrom (4 args) selector.
function test_validSafeTransferFrom4argsIntegration() public {
uint256 initialRecipientBalance_ = token.balanceOf(address(users.bob.deleGator));
address initialOwner_ = token.ownerOf(TOKEN_ID);
assertEq(initialOwner_, address(users.alice.deleGator), "Initial owner should be Alice");

Execution memory execution_ = Execution({
target: address(token),
value: 0,
callData: abi.encodeWithSignature(
"safeTransferFrom(address,address,uint256,bytes)",
address(users.alice.deleGator),
address(users.bob.deleGator),
TOKEN_ID,
hex""
)
});
Caveat[] memory caveats_ = new Caveat[](1);
caveats_[0] =
Caveat({ args: hex"", enforcer: address(erc721TransferEnforcer), terms: abi.encodePacked(address(token), TOKEN_ID) });
Delegation memory delegation_ = Delegation({
delegate: address(users.bob.deleGator),
delegator: address(users.alice.deleGator),
authority: ROOT_AUTHORITY,
caveats: caveats_,
salt: 0,
signature: hex""
});
delegation_ = signDelegation(users.alice, delegation_);
Delegation[] memory delegations_ = new Delegation[](1);
delegations_[0] = delegation_;
// Execute Bob's UserOp with safeTransferFrom (4 args)
invokeDelegation_UserOp(users.bob, delegations_, execution_);

uint256 finalRecipientBalance_ = token.balanceOf(address(users.bob.deleGator));
address finalOwner_ = token.ownerOf(TOKEN_ID);
assertEq(finalRecipientBalance_, initialRecipientBalance_ + 1, "Recipient balance should increase by 1");
assertEq(finalOwner_, address(users.bob.deleGator), "Token owner should be Bob");
}

function _getEnforcer() internal view override returns (ICaveatEnforcer) {
Expand Down