reNFT - BARW's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L733-L776 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L265-L285

Vulnerability details

Impact

Since the creation of a rental consumes less gas per rental NFT than stopping a rental, creating a rental with a lot of NFTs might be possible but stopping it might run out of gas. Not being able to stop a rental will lock the rented NFTs in the borrowers safe and prevent the lender to ever being paid.

Proof of Concept

To create a new rental the function Create:validateOrder() is called, to stop a rental Stop::stopRental() is called. Tests show that when creating a rental, every additional NFT added to the rental increases the gas cost of the transaction by at least 29,000 gas. When stopping a rental, every additional NFT adds at least 36,200 gas to the transaction.

For convenience here is a table of the gas cost per additional NFT for different numbers of NFTs in a rental:

As can be seen in the table above, creating a rental of 401 NFTs would be possible but stopping the rental would cost over 15 million gas. This is more than the gas block limit of Avalanche, one of the chains the reNFT protocol will be deployed on and will lead to an OOG revert. Therfore the rental can never be stopped and the rented NFTs will be stuck in the borrowers safe and the lender will never get paid.

Even though the number of 401 NFTs is quite high, it is not impossible to reach, depending on the game and the NFTs that are rented (e.g cosmetics for land plots in an “Axie Infinity Land” kind of game). Also, this calculation is not considering any additional gas used for onStop hooks or multiple ERC20 tokens for payment. Such additional gas cost could dramatically reduce the number of NFTs needed for an OOG event.

Following is a test that can be added to StopRent.t.sol to check the gas cost for calling Create: validateOrder() and Stop::stopRental. To be able to run the test the number of tokens deployed must be changed in test/fixtures/protocol/AccountCreator.sol::setUp from 3 to 500:

        // deploy 3 erc20 tokens, 3 erc721 tokens, and 3 erc1155 tokens
-         _deployTokens(3);
+        _deployTokens(1000);

In the test itself, the number of NFTs in the rental can be changed by adjusting the numberOfNfts variable in the beginning of the test. To run the test run:

forge test -vvv --gas-report --mt test_OOG_StopOrder

In the gas report look for the gas cost for the call to Create:validateOrder() and Stop::stopRental.

function test_OOG_StopOrder() public {
        uint256 numberOfNfts = 100;

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

        (
            Order memory order,
            bytes32 orderHash,
            OrderMetadata memory metadata
        ) = finalizeOrder();

        createOrderFulfillment({
            _fulfiller: bob,
            order: order,
            orderHash: orderHash,
            metadata: metadata
        });

        // finalize the base order fulfillment
        uint256 gasBefore4 = gasleft();
        

        RentalOrder memory rentalOrder = finalizeBaseOrderFulfillment();

        uint256 gasAfter4 = gasleft();
        console.log("Gas for creating rental", gasBefore4 - gasAfter4);

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

        // stop the rental order
        vm.prank(alice.addr);
        uint256 gasBefore5 = gasleft();
      
        stop.stopRent(rentalOrder);


    }

Tools Used

Manual review Foundry

Recommended Mitigation Steps

To ensure all rentals can be stopped consider introduceing a cap for the number of NFTs that can be added to a single rental. If a user want to rent out more NFTs he can still create multiple rentals

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

A lender can not add a hook to his rental that should only be executed when the rental is stopped, because creating the rental will fail.

Proof of Concept

When a new rental is created, Create::validateOrder is called with the ZoneParameters. This ZoneParameters include a RentalPayload which again includes the OrderMetadata. The order meta data includes, among other things, an array of Hooks that should be added to the rental order. This hook are executed when the rental order is created (onStart) or when the rental order is stopped (onStop). The Storage holds a whitelist of all hooks that can be added to a rental order. Hooks can be whitelisted to be onStart or onStop or both.

The problem arises in the function Create::_addHooks that is called during the creation of a rental. This function loops over all provided hooks and checks if they are whitelisted as onStart. If not, the function reverts:

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

According to the team a hook that is only supposed to be executed onStop, will not be whitelisted as onStart. This means that if a lender would like to add such a only onStop hook to his rental, the creation of the rental will revert.

Tools Used

Manual review

Recommended Mitigation Steps

During the creation of a rental, when checking the hooks a lender wants to add, consider not reverting on hooks that are not whitelisted as onStart but also checking if the hook is whitelisted as onStop. If so, skip the execution part of the loop and continue with the next hook. If the loop is neither whitelisted as onStart nor onStop, revert the function.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If a lender has added a hook to his rental that should only be executed onStart, it will not be possible to stop the rental because the call to Stop::stopRent will always fail. This will result in the rented NFTs beeing stuck in the borrowers safe and the lender never receiving his payment for the rent.

Proof of Concept

When a rental is stopped, Stop::stopRent is called with the RentalOrder data of the rental that should be stopped.
Part of the RentalOrder data is an array of hooks that were added to the rental. This hook are executed when the rental order is created (onStart) or when the rental order is stopped (onStop). The Storage holds a whitelist of all hooks that can be added to a rental order. Hooks can be whitelisted to be only onStart or only onStop or on onStart and onStop.

The problem arises in the function Stop::_removeHooks that is called during the process of stopping a rental. This function loops over all provided hooks and checks if they are whitelisted as onStop. If not, the function reverts:

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

According to the team, a hook that is only supposed to be executed onStart, will not be whitelisted as onStop. This means that if a lender added a hook to his rental that is only onStart, stopping the rental will not be possible since the call to Stop::_removeHooks will always revert because of the check shown above. This will leave the rented items stuck in the safe of the borrower and prevent the lender being payed for the rental.

Tools Used

Manual review

Recommended Mitigation Steps

During the process of stopping a rental, when checking the hooks that have been added to the rental, consider not reverting on hooks that are not whitelisted as onStop but also check if they are whitelisted as onStart. If a hook is not whitelisted as onStop but is whitelisted as onStart, skip the execution part of the loop and continue with checking the next hook. If the loop is neither whitelisted as onStart nor onStop, revert the function.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

If a lender adds a hook that should be executed onStop, but this hook is removed from the whitelist during the rental time, it will not be possible to stop the rental since the call to Stop::stopRent will fail. This will result in the rented NFTs beeing stuck in the borrowers safe and the lender never receiving his payment for the rent.

Proof of Concept

When a rental is stopped, Stop::stopRent is called with the RentalOrder data of the rental.
Part of the RentalOrder data is an array of hooks that were added to the rental when it was created. This hook can be executed when the rental order is created (onStart) or when the rental order is stopped (onStop). The Storage holds a whitelist of all hooks that should be executed onStart and onStop. Hooks that were added to the whitelist can also be removed from it.

The problem arises in the function Stop::_removeHooks that is called during the process of stopping a rental. This function loops over all provided hooks and checks if they are whitelisted as onStop. If not, the function reverts:

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

If a hook was whitelisted as onStop when the rental was created but was removed from the whitelist during the rental time, this will prevent the rental to be stopped since the function will always revert. The result would be the rented NFTs beeing stuck in the safe of the borrower and the lender never getting his payment.

Tools Used

Manual review

Recommended Mitigation Steps

During the process of stopping a rental, when checking the hooks that have been added to the rental, consider not reverting on hooks that are not whitelisted as onStop since they might have been removed from the whitelist during the rental period. Instead just skip the execution part of the loop and continue with checking the next hook.

Assessed type

Timing

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L226-L248

Vulnerability details

Impact

If a lender adds a hook that should be executed onStop, but the execution of this hook fails for any reason that cannot be fixed, it will not be possible to ever stop the rental since the call to Stop::stopRent will fail. This will result in the rented NFTs being stuck in the borrowers safe and the lender never receiving his payment for the rent.

Proof of Concept

When a rental is stopped, Stop::stopRent is called with the RentalOrder data of the rental.
Part of the RentalOrder data is an array of hooks that were added to the rental when it was created. This hook can be executed when the rental order is created (onStart) or when the rental order is stopped (onStop). All hooks that are onStop need to be executed when stopping a rental.

If the execution of a hook fails, the function reverts. If the reason why the hook failed can not be fixed, there is no way to skip the execution of this hook. In such a case the rental can never be stopped, the rented NFTs will be stuck in the safe of the borrower and the lender will never get paid.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a “blacklist” of hooks to the Storage that holds hook that should be skipped when stopping a rental. Check this blacklist before executing any onStop hook. This would allow the admin to add failing hook that cannot be fixed to the blacklist and enable lenders to stop their rent even if they had added such a failing hooks when creating the rental.

Assessed type

DoS

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L457-L490

Vulnerability details

Impact

All gnosis safes launched by the Factory are given the Stop policy as a module and the Guard policy as a guard. If the Stop policy is ever deactivated, all NFTs will be stuck in the safes where the Stop policy is set as a module since it will not be possible to call stopRent anymore.

Proof of Concept

For a user to be able to rent NFTs through the reNFT protocol, he first needs to create a gonsis safe by calling the function deployRentalSafe in the factory. By doing this, a new safe is deployed and the Stop policy is set as a module for the safe and the Guard policy is set as the guard for the safe. This is done to ensure that the rented NFTs are not transferred out of the safe during the rental phase.

For this purpose, all transactions from the safe are checked by the guard to ensure that they do not contain forbidden actions like approving or transferring NFTs, changing the guard or activating or deactivating new modules.

The Stop policy on the other hand has the appropriate roles to be able call Escrow and State and thereby to stop a rental and move the ERC20 tokens and NFTs associated with the rental around.

The problem arises if the Stop policy is ever deactivated by the executor through the Kernal. If this is the case, the Stop policy will loose all its rights to interact with the Escrow and State contracts and it will not be possible anymore to stop a rental, move the NFTs back to the lender and pay the lender. Mathers are made worse by the fact that no new Stop policy can be activated for the gnosis safe since this would be prevented by the guard. If the Stop policy is ever deactivated, all gnosis saves that were deployed by the factory will be affected.

Tools Used

Manual review

Recommended Mitigation Steps

To prevent the issue described above consider creating a whitelist of policies that can never be deactivated in the kernal. Check this whitelist when _deactivatePolicy is called to ensure to not deactivate a crucial policy.

Assessed type

Access Control

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/modules/PaymentEscrow.sol#L190-L202 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L265-L306

Vulnerability details

Impact

In certain scenarios, if a borrower disables the ERC777 transfer with tokensReceived, the process can be reversed on tokensReceived. This action can consequently initiate a chain of reverts.

1. PaymentEscrow._settlePaymentInFull revert
2. PaymentEscrow._settlePayment revert
3. PaymentEscrow.settlePayment revert
4. Stop.stopRent revert Consequently, the borrower's PAY rentals can't be halted, allowing the borrower to retain the NFT indefinitely.

Proof of Concept

1. The attacker create an attack contract with tokensReceived
2. The attacker borrows an NFT with attack contract
3. The rental process becomes unstoppable due to tokensReceived

Tools Used

Foundry

Recommended Mitigation Steps

Let user claim their fee instead of send it to user address.

Assessed type

ERC20