reNFT - 0xA5DF's results

Lines of code

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

Vulnerability details

Impact

When rental is active no tokens of the type (same address and same token ID) of the rented tokens can be transferred. This means that an owner of a safe can use a rental to lock in non-rented tokens that exists in the safe.

Proof of Concept

Consider the following scenario:

• A group of friends are using the safe for renting an ERC721 token
• They also store another ERC1155 token in the safe
• One of the owners isn’t happy with what the group intends to do with the ERC1155 tokens so they create an order of 1 wei for 1000 years ahead of the same ERC1155 token
• Since the attacker is one of the owners the order goes through
• Now all of the ERC1155 in the contract is stuck forever

Recommended Mitigation Steps

Either:

• Allow withdrawing tokens from safe as long as the remaining balance is greater than the amount of locked tokens
• Require that the safe would initiate the order fulfillment, this way we can ensure the rent is with agreement of all owners

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Rental stop reverts if one of the specified hooks is disabled. The hooks are specified during rental creation, so if the admins disable the hooks in the meanwhile the rental wouldn't be stopped. This would deprive the lender/renter from payment and the lender from getting back their rented tokens.

Another scenario where this is possible, is if a hook is enabled only for rental-start, in that case the rental can start but can't be stopped.

Proof of Concept

During rental stop the hooks are checked, if the hook is disabled the function would revert.

            if (!STORE.hookOnStop(target)) {
                revert Errors.Shared_DisabledHook(target);
            }

Hooks can be disabled by the admins at any time: code:

    /**
     * @notice Updates a hook with a bitmap that indicates its active functionality.
     *         A valid bitmap is any decimal value that is less than or equal
     *         to 7 (0x111).
     *
     * @param hook   Address of the hook contract.
     * @param bitmap Decimal value that defines the active functionality on the hook.
     */
    function updateHookStatus(
        address hook,
        uint8 bitmap
    ) external onlyByProxy permissioned {

Hooks list is shared between rental start and stop, so when a lender specifies hooks they'd be used for both start and stop. code

            // Generate the rental order.
            RentalOrder memory order = RentalOrder({
                seaportOrderHash: seaportPayload.orderHash,
                items: items,
                hooks: payload.metadata.hooks,
                orderType: payload.metadata.orderType,
                lender: seaportPayload.offerer,
                renter: payload.intendedFulfiller,
                rentalWallet: payload.fulfillment.recipient,
                startTimestamp: block.timestamp,
                endTimestamp: block.timestamp + payload.metadata.rentDuration
            });

Recommended Mitigation Steps

Either:

• Skip disabled hooks (don't call them but don't revert)
• Skip the check and run the hooks anyways, because during creation they were allowed (don't keep a separate status for start and stop hooks in that case)

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L27-L50

Vulnerability details

Impact

The lender can revert the rental stop by reverting the onXReceived() function calls. This would deprive the renter from their payment (in case of a PAY order). Additional effect might be if the rental includes ERC1155 tokens and the renter holds some identical non-rented ERC1155 in the safe - preventing the rental stop would prevent the transfer of those non-rented tokens.

Proof of Concept

Consider the following scenario:

• Bob creates a PAY order via a safe they own (i.e. the lender is actually a safe, controlled by Bob)
• Alice fulfills the order
• During the rental the value of the NFT has dropped to nearly zero, so Bob doesn't care about it anymore
• Before the rental ends Bob disables the onERC721Received() function on the safe, so that any call to it would revert
• Now any attempt to stop the rental would revert. Bob can deprive Alice from the rental payment as long as he'd like.

Recommended Mitigation Steps

In case that the attempt to transfer reverts - create a safe with the lender as the sole owner and transfer the tokens to there.

Assessed type

Error

Judge has assessed an item in Issue #330 as 2 risk. The relevant finding follows:

L5

L1 - Factory.deployRentalSafe() can be front-run to revert safe creation

Consider the following scenario:

• A user sends out a tx to call Factory.deployRentalSafe()
• The attacker sees that and front runs it, calling SafeProxyFactory.createProxyWithNonce() directly with the same parameters
• The user's tx would revert because the safe already exists

The user can mitigate that by first creating a safe with a different set or order of owners, or a different threshold, and then creating the safe they wished to create (the attacker can still keep front-running those txs though).

A way to mitigate that (prevent front running) is to deploy a fork of SafeProxyFactory with access control - allowing only the protocol's Factory to create new safes.

L2 - initializer can be front run

Both modules are using a proxy with initializers, those initializers can be front run during deployment. As mitigation - ensure that initialization runs at the same tx with deployment.

L3 - Renter can steal CryptoKitties token using transfer()

CryptoKitties uses a non-standard ERC721 interface, this means that in case it's rented it can be stolen by the renter since the Guard can't stop it. The collection is pretty popular, and it can be traded on Seaport using Conduit. This might be classified as a known issue, but given that CryptoKitties is commonly - it's worth noting that.

L4 - On transaction hooks aren’t controlled by the lender

Contrary to the docs which state that the hooks are controlled by the lender - in reality all active on-transaction hooks run regardless of what the lender has specified in the order.

L5 - impossible to disable modules that has been removed from whitelist

Consider the following scenario:

• Module has been whitelisted
• Some safes add it
• Module has been removed from whitelist
• Safes can’t disable this module now

Mitigation - either make a separate allowlist for disabling, or allow disabling all modules but the Stop module

L6 - limit the rental to a reasonable time, to prevent creating for an unreasonable time by mistake

Currently it seems that the protocol allows rental for thousands of years, that doesn’t make much sense and if a user specifies that by mistake that might lock their tokens forever.

L7 - if stop module is whitelisted it can be disabled by the safe

If stop module is whitelisted by mistake - renters can just disable it and prevent rental stop. As mitigation, consider adding a check during whitelisting to prevent whitelisting it.

L8 - Can’t update the status of a self destructed hook

The protocol doesn't allow updating the status of hooks with no code. The issue is that if a hook is self destructed for any reason the admin can't update its status any more.

code

        if (hook.code.length == 0) revert Errors.StorageModule_NotContract(hook);

As mitigation - if the hook doesn't have any code allow only disabling it (setting the status to zero).

L9 - if stop policy is replaced it’d brick the rental stop for older safes

The stop policy can be replaced via the kernel, if the old stop policy is disabled it'd be impossible to stop the rentals - since only the old stop policy has access to the safe.

As mitigation - either keep the old stop policy active, or add some migration mechanism (allowing the old stop policy to add the new stop policy to the safe as a module. This has to be done before deploying the current stop policy)

R1 - Accumulator._insert() might result in memory collision in future code

This function expands the size of the rentalAssets memory variable, assuming there's no memory that's currently used afterwards. This is true under current code, but worth keeping an eye on it for future code changes or upgrades - if there's another memory variable after rentalAssets when this function is called it'd override it, causing severe issues.

Code

            // Update the free memory pointer so that memory is safe
            // once we stop doing dynamic memory array inserts
            mstore(0x40, add(newItemPosition, 0x40))

R2 - impossible to remove a hook from a specific contract

While it’s possible to disable a hook, that would disable it for all contracts that are using it. If we want to enable it on one target and remove it from another target that’s impossible, because we can’t set it back to the zero address (since the zero address doesn’t have any code). As mitigation, allow setting it to the zero address.

code

        // Require that the `hook` address is a contract.
        if (hook.code.length == 0) revert Errors.StorageModule_NotContract(hook);

Centralization risks

Trusted roles:

Overall it seems that most roles here hold a very high amount of power over the system and compromising them would cause loss of funds for the protocol and the users. Therefore, all of them must be behind a timelock to allow users to withdraw their funds in case one of the roles gets compromised (with an exception of CREATE_SIGNER and escrow.skim(). The latter is protocol fees and won’t harm the users). On top of that, given that the tokens are locked in the contracts till the end of the rental a timelock on its own wouldn’t help for orders which are longer than the timelock. I’d suggest adding an escape feature, where with an agreement of both the lender and the renter the rent can end and the tokens can be extracted safely from the safe and escrow. There'd still be a risk of the admin collaborating with the renter or lender, but that's a much smaller risk. (an alternative might be to require the rentals to be shorter than the timelock delay, but that doesn’t seem very reasonable)

Limiting the fee level

On top of that, it'd make sense to set a max fee, to limit the amount of fee that the owner can set, esp. considering that under current situation the fee applies retroactively.

Pausing mechanism

Currently there's no pausing mechanism in the protocol. However, all orders have to be signed via the CREATE_SIGNER role, that makes it possible for the admins to pause the creation of new orders by not signing orders. Given that the intention of a pausing mechanism is to stop new activity (but allowing the completion of existing activity) this might be sufficient. The rest of the actions in the system (safe tx during rental, stopping rentals) are for rents that already begun and it wouldn't make sense to give the pauser the power to pause them.

It's worth mentioning though that given that the CERATE_SIGNER needs to constantly sign orders it would have to be a warm wallet, which put it at a higher risk of being compromised. Therefore it might be worth to add additional pause mechanism, or make the admin of the CERATE_SIGNER role an EOA or multisig without a timelock mechanism, this way the CERATE_SIGNER role can be revoked immediately if it has been compromised.

Default framework

Access control

Access control to the modules is managed by the kernel. I've verified that all external/public functions in the modules have the permissioned modifier which would ensure that it can only be accessed with a policy with the right permissions.

Dual upgrade mechanism

Besides the framework's feature to update a module we can also upgrade the current modules implementation via the Admin policy. This makes sense, since an upgrade via the kernel would mean changing to a different address which wouldn't preserve the storage of the previous address. However, it's worth keeping an eye on the fact that we have another way to upgrade the modules which might be at the hands of a different role.

Integration with contracts not in scope

Future code

The following types contracts are to be added to the protocol in the future, besides failing to do what they're supposed to do they bare some additional risks:

Note that bypassing the guard means you can easily steal the rented items, or even remove the guard completely.

External libraries

Other contracts excluded from scope

Contracts with little code

The following contracts have been excluded from scope:

• src/packages/Zone.sol
• src/libraries/KernelUtils.sol
• src/libraries/RentalUtils.sol
• src/proxy/Proxiable.sol
• src/proxy/Proxy.sol

Most of them contain rather simple logic so the chance of having bugs in them is pretty low, still it's worth auditing those since the chance of them having bugs isn't zero.

Interfaces, error, events and structs

Those don't contain any logic, so the chance of them having bugs is really low.

Critical parts of the protocol

Rental creation and order validation

• Ensure only Seaport can call the validateOrder() function
• validateOrder() should ensure:
  • Items are sent to the right addresses
  • The CREATE_SIGNER signed the rent payload
  • Rental is registered to storage

Guarding against rented token escape

The most difficult part here is preventing the escape of a rented token from a safe. There might be many different ways to transfer a token and it might be difficult to try to block them all.

Pre approving a token

We can try to bypass the guard by approving the tokens before the rental begins, however this doesn't seem to work:

• As for ERC721 all specific approves are deleted during transfer
  • This is according to ERC721 specs:

    ///  When a Transfer event emits, this also indicates that the approved address for that NFT (if any) is reset to none.    
    

• ERC1155 doesn't have a function to approve a specific token
• approveForAll() isn't allowed at all times

Approving for a previous 'life' of the safe

Technically we can deploy a safe, approve tokens, destroy it and then create another safe. However it doesn't seem to work in this case, since in order to deploy it to the same address we have to deploy it with the same parameters, which means it'd be with the same guard that would stop approving and destroying the safe.

Running before the guard is set

The guard is set right when the safe is created, there's no wiggle for an attacker to run any function before it's set.

Weird ERC721/ERC1155 tokens

This is where the biggest risk lays, tokens often take their freedom in adding new features to their own implementation. Those cool 'features' might be used to bypass the guard. The sponsor listed this as a known issue, but it's worth noting that these might be tokens which are commonly used (e.g. crypto kitties), and the average users wouldn't expect them to cause issues when using them in the protocol.

• Alternative functions to transfer tokens
  • E.g. another parameter to add some metadata / additional instructions
• Alternative functions to approve
  • E.g. ERC1155 function to approve a specific token
• Different behavior for existing functions
  • E.g. not erasing the approval of ERC721 on transfer

Rental stop

• Ensuring the rental exists and has ended
  • Deleting the rental from storage / Guarding against double rental stop
• Transferring payment from escrow to lender (or renter for PAY order)
• Transferring ERC721/1155 tokens back to the lender

Reentrancy

onXReceived

This seems the obvious way to achieve reentrancy here. During rental start the tokens are transferred to the safe, the onXReceived functions in the fallback handler of the safe simply return and do nothing.

During rental stop the tokens are transferred back to the lender and this is where we can achieve reentrancy.

• Attempting to reenter and stop the same order again would probably revert during the check in Storage
• Attempting to reenter and stop another rental would probably not have any effect on current rental or vice versa
  • The 2 interaction that weren't done yet during the reentrancy are the settle payments and marking the rental as removed in storage. Both don't seem to be affected by another rental being stopped before they run (or them affecting the other order).
• Attempting to create another order during that time
  • It wouldn't have the same order hash, as the hash is based on the rental start timestamp as well

Despite that, it might be good not to put a reentrancy guard, or at least to move the _reclaimRentedItems() interaction to the bottom of the function.

During safe tx

The safe's Guard is pretty stateless - all txs are either allowed or prohibited, therefore it doesn't seem that a reentrancy can cause any issues with current code. As for future code - hooks, extensions and delegate calls - it's worth taking that into account when auditing the code.

Effects of the protocol over other tokens in the safe

Safes created by the protocol can be used to store other tokens. However the safe would still be restricted by the guard:

• Delegate calls are restricted to allowlist
• Approve for all is not allowed
• Modules are restricted to allowlist
• Non-rented ERC1155 tokens can't be transferred out of the safe if there's a similar token (same address, same token ID) that's rented.
• The protocol's modules can transfer tokens out of the safe without the owner's approval. It's important to communicate that to the user, esp. the first effect.

Order hash uniqueness

The orderHash used in the protocol is a hash of the RentalOrder struct which contains Seaport's orderHash. This means that as long as Seaport's order hash is unique then the protocol's order hash would be unique as well (I didn't dive deep enough into Seaport to check that, but it makes sense that this is the case). Still, it might be worth for additional security to verify during registration that the order hash wasn't registered yet.

Push0 opcode on L2s

Some L2s might not support the push0 opcode, given that the compiler version is greater than 0.8.20 then it might cause issues if compiled to the Shanghai evm. I noticed the foundry.toml targets the paris evm for that reason, so as long as it's compiled for that evm this shouldn't be an issue.

Time spent:

25 hours