Lines of code

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

Vulnerability details

Impact

Some ERC-721 projects do implement external burn methods that allow token owners to burn their own NFTs. Note that this doesn't violate ERC-721 standard, and neither is it considered malicious.

When the offered assets of an order contains an ERC-721/ERC-1155 token that has an external burn function that allows token owner to burn their own NFTs, the NFT that is supposed to be locked in the rental safe can be burned before rental order expires, causing the lender(offerer) losing their NFT permanently.

In addition, when the rental order is BaseOrder type, the lender(offerer) will also lose their ERC20 payment permanently.

Proof of Concept

Guard.sol is a policy contract that is set as a guard for all rental safe contract(Safe.sol) deployed. Guard.sol will run checkTransactions() for any execution from the order fulfiller's rental safe.

When a rental order is created in reNFT, the offer assets (ERC721/ERC1155) will be transferred and 'locked' in the fulfiller(renter)'s rental safe contract until rental is stopped by lender or expires. This locking mechanism is enforced through Guard.sol by checking the function signatures to be executed from safe and reverting when sensitive function signatures such as e721_safe_transfer_from_1_selector,e721_safe_transfer_from_2_selector, etc are part of the calldata.

However, the vulnerability is that Guard.sol's checkTransactions() only check for transfer related function signatures but doesn't check for burn related signatures. This allows any ERC721/ERC1155 that implements external burn methods to be burned freely from the rental safe contract.

Note that even through READ_ME excluded Dishonest ERC721/ERC1155 Implementations that adds an additional function to transfer the token to another wallet. An ERC721 that implements an external burn function to allow token owner to burn their own NFT is not considered dishonest, and neither does it violate ERC721 spec. Therefore, Guard.sol should consider including checks for burn related token methods to ensure that NFT's locked and protected in fulfiller's safe contract.

//src/policies/Guard.sol
    function _checkTransaction(address from, address to, bytes memory data) private view {
...
     if (selector == e721_safe_transfer_from_1_selector) {
...
            // Check if the selector is allowed.
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
    } else if (selector == e721_safe_transfer_from_2_selector) {
...
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_transfer_from_selector) {
...
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
        } else if (selector == e721_approve_selector) {
...
            _revertSelectorOnActiveRental(selector, from, to, tokenId);
...
        } else {
...
           //@audit There is no checking whether the selector is burn or burnBatch
           if (selector == shared_set_approval_for_all_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
...
            if (selector == e1155_safe_batch_transfer_from_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
...
            if (selector == gnosis_safe_set_guard_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L270-L287)

As seen above, Guard.sol's _checkTransaction() allows it when a rental safe is executing a transaction that either calls burn() on an ERC721 or ERC1155 that allows the token owner to burn their token. The fulfiller who's the owner of the rental safe can execute transactions from the safe to burn their ERC721/ERC1155 asset. This way, when the rental order is stopped by the lender, the NFT will not be transferred back to the lender.

Suppose that the rentalOrder is BaseOrder type. A base order can only be stopped when the rental duration passes, when the lender call Stop.sol - stopRent() to stop rental, two transfers are supposed to take place: (1) the offered assets (ERC721/ERC1155) 'locked' in the fulfiller's rental safe will be transferred to the lender; (2) the ERC20 payments sent by the fulfiller at order creation and now stored in PaymentEscrow proxy will be transferred to the lender as well.

When the fulfiller already burned the ERC721 asset in their rental safe when stopRent() is called. (1) will revert. Due to the rental safe will make a delegate call to Recalimer.sol - reclaimRentalOder() which will call IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier). And this will revert due to item.identifier (tokenId) has already been burned by the fulfiller.

In addition, (2) will also revert, which causes the lender losing both their NFT asset and their ERC20 payments from the fulfiller.

Note that the fulfiller can choose to burn the lender's NFT at or close to the rental expiration timestamp, such that fulfiller will get the full value of rented NFT.

//src/packages/Reclaimer.sol
    function reclaimRentalOrder(RentalOrder calldata rentalOrder) external {
...
//@audit-info note: this is a delegate call, _transferERC721() will transfer ERC721 out of safe contract back to the lender
            if (item.itemType == ItemType.ERC721)
                _transferERC721(item, rentalOrder.lender);
...
}
    function _transferERC721(Item memory item, address recipient) private {
//@audit when the fulfiller's safe contract already burned the token, this will revert, and DOS Stop.sol - stopRent() flow, this will also revert settlePayment flow from the PaymentEscrow
|>      IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

(https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L95)

Tools Used

Manual Review

Recommended Mitigation Steps

In Guard.sol, _checkTransaction() also add additional check for burn() selectors to prevent malicious asset token burning from the fulfiller.

Assessed type

Other

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

Standard ERC721/ERC1155 with external burn function, will bypass Guard.sol's check allowing the fulfiller to maliciously burn the asset borrowed from the offerer #247