reNFT - Qkite's results

Lines of code

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

Vulnerability details

Impact

Under Signer, there is an instance of incorrect encoding of hashes, where there is a discrepancy between the string and how the hash is encoded.

Proof of Concept

The function _deriveRentalOrderHash returns a keccak256 hash of the order hashed with _RENTAL_ORDER_TYPEHASH. However, there is a difference between the _RENTAL_ORDER_TYPEHASH string and the order that _deriveRentalOrderHash hashes.

return keccak256(
    abi.encode(
        _RENTAL_ORDER_TYPEHASH,
        order.seaportOrderHash,
        keccak256(abi.encodePacked(itemHashes)), 
        keccak256(abi.encodePacked(hookHashes)),
        order.orderType,
        order.lender,
        order.renter, 
        //@audit `address rentalWallet` missing here
        order.startTimestamp,
        order.endTimestamp
    )
);

_RENTAL_ORDER_TYPEHASH is derived from _deriveRentalTypehashes, where rentalOrderTypeHash combines rentalOrderTypeString with a few other strings. For now, we are interested in rentalOrderTypeString, which contains the following:

bytes memory rentalOrderTypeString = abi.encodePacked( 
    "..., address renter, address rentalWallet, uint256 startTimestamp, uint256 endTimestamp)"
        );

From the string, we can conclude that there should be address renter followed by address rentalWallet and so on. However, in _deriveRentalOrderHash, after order.renter, there is order.startTimestamp, and address rentalWallet is entirely skipped in this case.

This discrepancy can cause issues with generating signatures, as the system is guided by the string, while the contract misses some crucial parts of it.

Tools Used

Manual review.

Recommended Mitigation Steps

To address this issue, add the rentalWallet address to the missing order hash.

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

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

_deriveRentalOrderHash fails to encode a crucial parameter - rentalWallet. This allows users to drain some safes while leaving NFTs stuck in others.

Proof of Concept

When users call stopRent, they enter their desired order. It passes a few checks, and the NFTs are sent to the lender (with _reclaimRentedItems), and the order is deleted (with removeRentals). When removing the order, removeRentals hashes it (using _deriveRentalOrderHash) and checks if it exists in the system.

However, the hash is incomplete, as it's missing one crucial part - rentalWallet.

return keccak256(
    abi.encode(
        _RENTAL_ORDER_TYPEHASH,
        order.seaportOrderHash,
        keccak256(abi.encodePacked(itemHashes)),
        keccak256(abi.encodePacked(hookHashes)),
        order.orderType,
        order.lender,
        order.renter,
        //@audit missing rentalWallet
        order.startTimestamp,
        order.endTimestamp
    )
);

This means that we can change rentalWallet (the address of the safe) value and still pass this existence check. This is where the exploit happens.

If there are two safes holding the same ERC1155 token with the same ID, we can swap their rentalWallet addresses when ending one's order. This will, in turn, drain the rentalWallet we used and remove the other order from the system, with its safe still full of the ERC1155s. This will cause the second safe to be bricked forever.

Example:

1. Alice rents from Bob 10 ERC1155 tokens with ID 5 (they can be items, pets, etc...).
2. Carol rents from Dan 1 ERC1155 token with ID 5.
3. Carol's order finishes.
4. Carol calls stopRent with her order but changes rentalWallet (safe) address to Alice's rentalWallet address.
5. The TX passes and removes 1 NFT from Alice's safe.
6. Alice's safe has 9 NFTs left, and 1 NFT is in Carol's wallet.
7. Carol's order gets deleted from the system, and her NFT remains stuck.
8. Alice cannot end her order as the order tries to withdraw 10, but there are only 9 in the safe.

POC

Gist - https://gist.github.com/0x3b33/7d928fc232596a2c6bb8d2770e92df5b Place in - smart-contracts/test/integration/<name>.t.sol Run with - forge test --match-test test_StopRentWithDifferentRentalWallet

Notice that we change 2 other contracts:

• Mock1155 - to mint us the same ID.
• OrderCreator - to make the 2 orders with the same NFT IDs.

Tools Used

Manual review.

Recommended Mitigation Steps

I would suggest including rentalWallet in _deriveRentalOrderHash's hash.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

_deriveOrderMetadataHash is missing uint8 orderType in its hash.

Proof of Concept

The orderMetadata string contains a few parameters, and uint8 orderType is one of them (orderMetadataTypeString -> orderMetadataTypeHash -> _ORDER_METADATA_TYPEHASH).

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

However, when _deriveOrderMetadataHash creates the metadata hash, it misses including orderType. This is not compliant with EIP721 and results in incorrect or incomplete hashes.

    function _deriveOrderMetadataHash(OrderMetadata memory metadata) internal view returns (bytes32) {
        bytes32[] memory hookHashes = new bytes32[](metadata.hooks.length);
        for (uint256 i = 0; i < metadata.hooks.length; ++i) {
            // Hash the hook
            hookHashes[i] = _deriveHookHash(metadata.hooks[i]);
        }

        // Derive and return the metadata hash as specified by EIP-712.
        //@audit missing `orderType`
        return keccak256(
            abi.encode(_ORDER_METADATA_TYPEHASH, metadata.rentDuration, keccak256(abi.encodePacked(hookHashes)))
        );
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Include orderType in the hash.

 -     return keccak256(abi.encode(_ORDER_METADATA_TYPEHASH, metadata.rentDuration, keccak256(abi.encodePacked(hookHashes)))
 +     return keccak256(abi.encode(_ORDER_METADATA_TYPEHASH, metadata.orderType, metadata.rentDuration, keccak256(abi.encodePacked(hookHashes)))
        );

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

The guard protects the safe owner from transferring the NFT away, however, it doesn't protect it from burn.

Proof of Concept

Some NFTs implement the burn extension or have burn as an external function (even the Mock721 has it). There is nothing stopping the renter from griefing the lender by simply burning the NFT. While this will not yield any profit (unless they are burning to mint another NFT), it allows them to cause damage to the lender.

Example:

1. Alice rents an NFT from Bob.
2. Alice calls safe.execTransaction with burn as data.
3. The NFT is burned.
4. When the rent expires, Bob calls stopRent, but his call reverts.

Bob cannot stop the order, and he can't even get the ERC20 that he rented the NFT for. This is because the NFT is burned, and safeTransferFrom will revert.

POC

Gist - https://gist.github.com/0x3b33/e805f909c6262f1e60274081be305685 Place in - smart-contracts/test/integration/<name>.sol Run with - forge test --match-test test_burnNFT -vv

Tools Used

Manual review.

Recommended Mitigation Steps

Add burn as another one of the selectors that the guard will check for.

     if (selector == shared_set_approval_for_all_selector) {
          revert Errors.GuardPolicy_UnauthorizedSelector(...);
     }

+    if (selector == e721_burn_selector){
+         revert Errors.GuardPolicy_UnauthorizedSelector(e721_burn_selector);
+    }
...

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

In pay orders, the lender pays the renter ERC20s to use their NFT. However, if the renter gets blacklisted for this ERC20, the NFT will remain stuck in the safe.

Proof of Concept

The stopRent function is called when an order is finished. It returns the NFTs and sends the payment to the renter (when a pay order is involved). However, if the renter somehow gets blacklisted from the payment token (USDC, USDT, etc.), the payment cannot be made. Since stopRent uses push over pull (via settlePayment -> _settlePayment -> _settlePaymentInFull), the entire transaction will revert. This will cause the NFT to remain in the safe.

Example:

1. Alice has 3 NFTs that she lends with a PAY order (paying USDC) to increase their levels.
2. Bob doesn't like Alice, so he rents those NFTs.
3. Bob gets blacklisted from USDC.
4. Alice tries to pull her NFTs, however, her transaction reverts.
5. The NFTs remain stuck in the safe.

Here, Bob doesn't gain anything, but he causes damage to the lender, leading to the loss of her 3 NFTs.

Tools Used

Manual review.

Recommended Mitigation Steps

I recommend using pull over push. In this case, stopRent should, instead of directly sending tokens, enable users to withdraw them from the escrow.

Assessed type

DoS