reNFT - trachev's results

Lines of code

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

Vulnerability details

Impact

Before presenting the issue I will give some background information on the stopping of a rental. When stopping a rental only one parameter is supplied to the stopRent function: RentalOrder calldata order. From this parameter, the same orderHash, supplied on rental creation, needs to be derived for stopRent to be called successfully. This is the data stored in an order's unique hash:

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
    )
);

One thing that it does not contain is the order.rentalWallet, which is where the problem arises.

Let's follow the code of stopRent to see where and how order.rentalWallet is used: 1/ Firstly it is used to get the rentalId of a rental item, which is then stored in the rentalAssetUpdates bytes array:

for (uint256 i; i < order.items.length; ++i) {
    if (order.items[i].isRental()) {
        // Insert the rental asset update into the dynamic array.
        _insert(
            rentalAssetUpdates,
            order.items[i].toRentalId(order.rentalWallet),
            order.items[i].amount
        );
    }
}

From this, a malicious actor can point to any rental item from any pending rental that has the same identifier and token address as the item that we are currently looping through. This is the data stored inside each rentalId: keccak256(abi.encodePacked(recipient, token, identifier)). Therefore, by only changing the recipient, which is the order.rentalWallet, to another rental wallet that also contains an amount of the same token, the malicious actor has gained access to a valid rental item that is not connected to the rental order they are stopping.

2/ After that, the order.rentalWallet is included in a call to _removeHooks. We can ignore this as many orders will not be making use of hooks and this will be skipped.

3/ Following that, reclaimRentedItems is called, where a call to the order.rentalWallet is made, which then calls reclaimRentalOrder. The rental wallet then sends the rented tokens to the order lender. The major issue is that as the rental wallet can be set to any wallet that has a balance of the rented tokens, the lender would be receiving assets that do not belong to them but to the victim's rental wallet. To summarise: rented tokens are sent not the lender that rented them out but to the lender of the order that is currently getting stopped, due to the wrong rental wallet that was supplied to the function initially.

4/ settlePayment is called which does not interact with the rental wallet so it is irrelevant to the issue.

5/ STORE.removeRentals is called which reduces the rentedAssets[asset.rentalId] balance of the victim's rental wallet that is supplied. As we said earlier the rentalId is pointing to a valid rented asset so as long as the attacked rental wallet has a high enough balance of the rented asset the attack will be successful.

To sum up, a rental wallet can be supplied to the order parameter of the stopRent function that is not connected to the actual order that is being stopped and is different from the rental wallet that was used when creating the rental. This occurs because the rental wallet is not included in the order hash and allows to steal the rented assets of another user's rental safe.

The impact of this attack would prevent the victim from stopping their own rental, causing them to lose all of their rented assets.

It is important to note that this attack will only function on ERC1155 tokens that are fungible as the attacked rental wallet needs to have a balance higher or equal to the rental wallet of the order that is being stopped.

Proof of Concept

Possible attack scenario: 1/ there are two pending rentals that are yet to be stopped 2/ the first one has 20 of an ERC1155 token and the other one has 100 of the same token 3/ The lender (can be the renter or any other user) of the first one is malicious and stops the rent but supplies the rental wallet of the second one 4/ 20 tokens are sent to the attacker and the victim's rental wallet has 80 tokens left that have become stuck, as the victim's rental will fail to be stopped 5/ The 20 tokens of the lender's actual rental wallet have also become stuck as the order hash that is connected to these assets is deleted

Tools Used

Manual review

Recommended Mitigation Steps

Include the order.rentalWallet to the order hash.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Hooks are contracts that may be included in an order, for them to be called during rental creation and the stopping of a rental. They all have a status that represents whether they can be used for gnosis safe transactions (hookOnTransaction), start of rental order (hookOnStart), stop of rental order (hookOnStop), or a combination of the three.

If a set of hooks are specified to be included in an order they all must have the status onStart. This validation can be found in the _addHooks internal function:

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

The array of hooks, used by a rental, is specified during its creation in the function validateOrder. They are also included in an order's unique hash which is essential when a rental is stopped, below we will see why this is really important.

When a rental is stopped stopRent is called with an only parameter RentalOrder calldata order. From this parameter, the same orderHash, used when creating the rental, must be derived in order for stopRent to not revert (this is checked in removeRentals in Storage.sol). Therefore, the same hooks specified during order creation must be specified during the stoppage of the rental.

The issue is that when a rental is stopped, the same hooks from its creation are called and they are all required to have the status hookOnStop, otherwise a revert is initiated:

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

Therefore, if a hook, specified in the rental creation, only has the hookOnStart status and does not have hookOnStop the rental will be created successfully and rental tokens will be transferred to the rental safe but when the rental is stopped the call would always fail as the hooks, specified during creation, do not have the hookOnStop status, causing all tokens to be stuck.

Proof of Concept

In the dev comments above the _removeHooks function it is written: @dev When a rental order is stopped, process each hook one by one but only if the hook's status is set to execute on a rental stop. This statement is incorrect as it assumes that the function will skip past hooks that are not for a rental stop and only process the ones that have that purpose. In reality, the function reverts on an invalid hook.

Tools Used

Manual review

Recommended Mitigation Steps

There are a few possible solutions depending on the protocol's intentions: 1/ on rental creation make sure that all hooks also have the hookOnStop status. 2/ on rental creation make sure that all hooks have either hookOnStop or hookOnStart and do the same check on a rental stop

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Currently, module contracts are proxy contracts that can be updated in two different ways. The first way is by setting a different implementation contract that the proxy will delegate calls to and the second is by updating the proxy itself through the _upgradeModule function on Kernel.sol.

There is no issue with the first way of updating the module functionalities as the data is stored on the proxy contract and will not be lost, but if the second way is used all of the previously stored data and assets in the Storage.sol and PaymentEscrow.sol contracts will be lost.

In particular, Storage.sol is used for storing the information of pending orders and rented assets, therefore if the module is upgraded, rentals that are not yet stopped will not be migrated to the new module, therefore they will never be fully completed, the rented tokens will not be returned to the lender, but instead they will be stuck in the rental wallet.

For PaymentEscrow.sol, the ERC20 consideration tokens that are stored there, in order to be sent to the lender once the rental process is over, will be stuck in the module and in the case of a PAY order the renter will not receive their payment.

Due to the major possible loss of funds, I have marked this issue as High.

Proof of Concept

    function _upgradeModule(Module newModule_) internal {
        // Get the keycode of the new module
        Keycode keycode = newModule_.KEYCODE();

        // Get the address of the old module
        Module oldModule = getModuleForKeycode[keycode];

        // Check that the old module contract exists, and that the old module
        // address is not the same as the new module
        if (address(oldModule) == address(0) || oldModule == newModule_) {
            revert Errors.Kernel_InvalidModuleUpgrade(keycode);
        }

        // The old module no longer points to the keycode.
        getKeycodeForModule[oldModule] = Keycode.wrap(bytes5(0));

        // The new module points to the keycode.
        getKeycodeForModule[newModule_] = keycode;

        // The keycode points to the new module.
        getModuleForKeycode[keycode] = newModule_;

        // Initialize the new module contract.
        newModule_.INIT();

        // Reconfigure policies so that all policies that depended on the old
        // module will refetch the new module address from the kernel.
        _reconfigurePolicies(keycode);
    }

As we can see the _upgradeModule function does not update only the implementation contract, but the proxy itself.

Tools Used

Manual review

Recommended Mitigation Steps

A short-term solution to the issue would be to disable the upgrade of modules. A long-term one would be to add a way of migrating the storage of the upgraded proxy to the new one.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The Guard.sol contract is used to make sure that a rental safe cannot transfer out rented tokens that belong to a lender. This validation is performed in the _checkTransaction and it validates that no malicious transfers or approvals can be performed.

The issue is that there is no prevention of burning tokens. Even though a burn function is not included in the ERC721 and ERC1155 it can be found in many popular NFT protocols. Therefore, any lender that decides to offer such tokens would be extremely vulnerable to losing them.

Furthermore, this cannot be included in the restrictions stated in the protocol's code4rena page that consider dishonest ERC721 and ERC1155 implementations as known issues, due to the fact that implementing a burn function is a standard procedure and does not equate to deviation from the ERC721 and ERC1155 standards.

Proof of Concept

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L203-L293 As we can see here there is no validation for making calls to a burn function, allowing any malicious renter to burn the lender's tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Include checks for malicious burning of tokens in _checkTransaction.

Assessed type

Invalid Validation

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L248-L262

Vulnerability details

Impact

As stated in a previous finding related to signature replay attacks when a rental is created through validateOrder the renter must provide a valid signature of the provided rental payload by the CREATE_SIGNER. The signature is used to make sure that the payload provided by the renter is not malicious and that the recipient of the rented assets is a rental safe. It is also used for the signer to reject fulfillments if the lender has requested the cancellation of their order.

The payload signed by the CREATE_SIGNER includes these values:

keccak256(
    abi.encode(
        _RENT_PAYLOAD_TYPEHASH,
        _deriveOrderFulfillmentHash(payload.fulfillment),
        _deriveOrderMetadataHash(payload.metadata),
        payload.expiration,
        payload.intendedFulfiller
    )
);

The issue here however will not be prevented by the inclusion of a nonce field.

In particular, there is no unique identifier of the specific order included in the rental payload. Therefore, a malicious renter is able to acquire a valid signature for one lender offer but instead of using it for fulfilling that offer and starting a rental, they would be able to use it for a different offer.

The following fields are included in each rental payload: 1/ _RENT_PAYLOAD_TYPEHASH - same for all payloads 2/ keccak256(abi.encode(_ORDER_FULFILLMENT_TYPEHASH, fulfillment.recipient)) - rental safe of renter 3/ keccak256(abi.encode(_ORDER_METADATA_TYPEHASH,metadata.rentDuration,keccak256(abi.encodePacked(hookHashes)))); - hashed hooks which are likely going to be the same for each offer of a lender 4/ signature expiration 5/ intended fulfiller - the malicious renter

Therefore, any signature, signed by the CREATE_SIGNER, can be used for any lender and for any payload that has the same rent duration and hooks.

Proof of Concept

Here is a possible scenario: 1/ renter attempts to fulfill a rental offer 2/ lender decides to cancel the offer off-chain by telling the CREATE_SIGNER to not sign that rental payload 3/ renter acquires a valid signature for a different offer and uses it instead on the offer that the lender wanted to cancel

Tools Used

Manual review

Recommended Mitigation Steps

Include the seaport.orderHash in the rental payload, signed by the CREATE_SIGNER.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

When a rental is created through validateOrder the renter must provide a valid signature of the provided rental payload by the CREATE_SIGNER. The signature is used to make sure that the payload provided by the renter is not malicious and that the recipient of the rented assets is a rental safe. It is also used for the signer to reject fulfillments if the lender has requested the cancellation of their order.

The payload signed by the CREATE_SIGNER includes these values:

keccak256(
    abi.encode(
        _RENT_PAYLOAD_TYPEHASH,
        _deriveOrderFulfillmentHash(payload.fulfillment),
        _deriveOrderMetadataHash(payload.metadata),
        payload.expiration,
        payload.intendedFulfiller
    )
);

The major issue is that there is no nonce, which is essential when preventing the replay of a CREATE_SIGNER signature. Therefore, once a renter has acquired a valid signature for a single order, they would be able to reuse it on other orders even if the lender has not agreed to the rental or decided to cancel it. The only requirement is that the rental payload of the malicious order is the same as the one that the CREATE_SIGNER signed initially.

Proof of Concept

This is the payload signed by the CREATE_SIGNER and as we can see there is no nonce field: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L248-L262

keccak256(
    abi.encode(
        _RENT_PAYLOAD_TYPEHASH,
        _deriveOrderFulfillmentHash(payload.fulfillment),
        _deriveOrderMetadataHash(payload.metadata),
        payload.expiration,
        payload.intendedFulfiller
    )
);

Tools Used

Manual review

Recommended Mitigation Steps

Include a nonce field to the rental payload and check to see if the nonce has already been used.

Assessed type

Invalid Validation