reNFT - CipherSleuths's results

Lines of code

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

Vulnerability details

Impact

if an order involves erc777 token for a pay order then in the tokensReceived callback the renter can create DOS situation resulting in the lender's assets being stuck in the rental safe

Proof of Concept

ERC777 token standard which is backward compatible with erc20 implies that on the transfer of the tokens the recipient can implement a tokensReceived hook to notify of any increment of the balance.

Now suppose a pay order is created with an erc 777 consideration asset as there is no restriction on that and also the eip specifies that

The difference for new contracts implementing ERC-20 is that tokensToSend and tokensReceived hooks take precedence over ERC-20. Even with an ERC-20 transfer and transferFrom call, the token contract MUST check via ERC-1820 if the from and the to address implement tokensToSend and tokensReceived hook respectively. If any hook is implemented, it MUST be called. Note that when calling ERC-20 transfer on a contract, if the contract does not implement tokensReceived, the transfer call SHOULD still be accepted even if this means the tokens will probably be locked.

so the tokensReceived hook is optional for a transfer/transferFrom call. Hence sending the assets from the lender's wallet to the escrow contract shouldn't be an issue.

Now when the rental period is over or in/between, the stopRent method in stop policy is called, which calls settlePayment in escrow module. Now on the token transfer

        (bool success, bytes memory data) = token.call(
            abi.encodeWithSelector(IERC20.transfer.selector, to, value)
        );

the tokensReceived hook if implemented by the renter, would be called and they could just revert the tx inside the tokensReceived hook which would mean that the assets lent by the lender are locked forever.

Tools Used

manual review

Recommended Mitigation Steps

It is recommended to prohibit erc777 tokens from being used as consideration items.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

_deriveOrderMetadataHash does not adhere to the eip712 standard by not considering all the encoded values in the orderMetadataTypeString & hence calculates the wrong order metadata hash

Proof of Concept

_deriveRentalTypehashes is responsible for calculating all rental related type hashes for enabling eip 712 signatures

orderMetadataTypeString has the following types that need to be taken into account when creating the orderMetadataTypeHash & OrderMetadataHash

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

but in _deriveOrderMetadataHash, orderType & emittedExtraData aren't used to calculate OrderMetadataHash which voilates the eip 712 specification and hence the wrong hash is calculated

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

This could be problematic if an off-chain tool uses a particular OrderMetadata struct for hash calculation using the correct EIP712 format thus creating a discrepancy between the reNFT calculated orderMetadataHash and off-chain calculated orderMetadataHash. Hence this could lead to numerous unintended behavior in the future when reNFT protocol expands and used by other off-chain tools.

eip712 clearly states that

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. Each encoded member value is exactly 32-byte long.

Tools Used

manual review

Recommended Mitigation Steps

update _deriveOrderMetadataHash calculation as follows

        return
            keccak256(
                abi.encode(
                    _ORDER_METADATA_TYPEHASH,
                    metadata.orderType,
                    metadata.rentDuration,
                    keccak256(abi.encodePacked(hookHashes)),
                    bytes32(metadata.emittedExtraData)
                )
            );

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L394-L400 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L389-L391

Vulnerability details

Impact

In the Signer._deriveRentalTypehashes function the rentPayloadTypeHash hash is derived as follows:

        rentPayloadTypeHash = keccak256(
            abi.encodePacked(
                rentPayloadTypeString,
                orderMetadataTypeString,
                orderFulfillmentTypeString
            )
        );

The RentPayload struct has two referenced structs namely OrderFulfillment and OrderMetadata.

The EIP712 states the below with respect to referenced structs inside a main struct when it comes to deriving the typeHash.

If the struct type references other struct types (and these in turn reference even more struct types), then the set of referenced struct types is collected, sorted by name and appended to the encoding. An example encoding is Transaction(Person from,Person to,Asset tx)Asset(address token,uint256 amount)Person(address wallet,string name).

As the EIP712 states the reference structs must be sorted by name and appended to the encoding. But in the rentPayloadTypeHash computation, the reference structs seem to be not sorted before concatenating to hash.

Hence the rentPayloadTypeHash typeHash computation does not follow the EIP 712 correctly. This could be problematic if an off-chain tool uses a particular rentPayload struct for hash calculation using the correct EIP712 format thus creating a discrepancy between the reNFT calculated rentPayloadTypeHash and off-chain calculated rentPayloadTypeHash. Hence this could lead to numerous unintended behavior in the future when reNFT protocol expands and used by other off-chain tools.

Proof of Concept

            rentPayloadTypeHash = keccak256(
                abi.encodePacked(
                    rentPayloadTypeString,
                    orderMetadataTypeString,
                    orderFulfillmentTypeString
                )
            );

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

            bytes memory rentPayloadTypeString = abi.encodePacked(
                "RentPayload(OrderFulfillment fulfillment,OrderMetadata metadata,uint256 expiration,address intendedFulfiller)"
            );

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to correct the typeHash calculation of the rentPayloadTypeHash according to the EIP712 standard as follows:

        rentPayloadTypeHash = keccak256(
            abi.encodePacked(
                rentPayloadTypeString,
                orderFulfillmentTypeString,
                orderMetadataTypeString
             )
        );

Here the reference struct types are sorted correctly and then appended for encoding and keccak256 hashing.

Assessed type

Other

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L406 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L384-L386 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L357-L359

Vulnerability details

Impact

The _ORDER_METADATA_TYPEHASH is calculated in the Signer._deriveRentalTypehashes as shown below:

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

As it is seen the type includes the Hook struct as a reference and it should be concatenated before calculating the typeHash as stated by the EIP712 standard shown below:

If the struct type references other struct types (and these in turn reference even more struct types), then the set of referenced struct types is collected, sorted by name and appended to the encoding.

But above EIP712 definition is not followed while computing the orderMetadataTypeHash as shown below:

            // Derive the OrderMetadata type hash using the corresponding type string.
            orderMetadataTypeHash = keccak256(orderMetadataTypeString);

The referenced struct type (Hook - "Hook(address target,uint256 itemIndex,bytes extraData)") is not appended in the encoding of the orderMetadataTypeHash. Hence the orderMetadataTypeHash does not follow the EIP712 standard while computing the typeHash.

This could be problematic if an off-chain tool uses a particular OrderMetadata struct for hash calculation using the correct EIP712 format thus creating a discrepancy between the reNFT calculated orderMetadataTypeHash and off-chain calculated orderMetadataTypeHash. Hence this could lead to numerous unintended behavior in the future when reNFT protocol expands and used by other off-chain tools.

Proof of Concept

            orderMetadataTypeHash = keccak256(orderMetadataTypeString);

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

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

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

        bytes memory hookTypeString = abi.encodePacked(
            "Hook(address target,uint256 itemIndex,bytes extraData)"
        );

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

The orderMetadataTypeHash computation should be corrected to follow the EIP712 standard as shown below:

            orderMetadataTypeHash = keccak256(abi.encodePacked( orderMetadataTypeString, hookTypeString));

Here the hookTypeString which is a referenced struct type is appended to the orderMetadataTypeString thus correctly following the EIP712 standard.

Assessed type

Other

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

L1,L5

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Stop.sol#L353 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L90-L100 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Reclaimer.sol#L32-L34

Vulnerability details

Impact

The Stop.stopRentBatch function is used to stop a batch of rentals by providing an array of RentalOrder structs to the function.

For each of the RentalOrders in the struct the Stop._reclaimRentedItems function is called as shown below:

            _reclaimRentedItems(orders[i]);

The _reclaimRentedItems function calls the Stop.reclaimRentalOrder function via a delegateCall from the Gnosis safe. In the reclaimRentalOrder function the ERC721 or ERC1155 is transferred to the lender for each of the Items as shown below:

        // Transfer each item if it is a rented asset.
        for (uint256 i = 0; i < itemCount; ++i) { //@audit-info - struct Item { ItemType itemType; SettleTo settleTo; address token; uint256 amount; uint256 identifier; }
            Item memory item = rentalOrder.items[i]; //@audit-info - cache each item

            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721) //@audit-info - enum ItemType { ERC721, ERC1155, ERC20 }
                _transferERC721(item, rentalOrder.lender); //@audit-info - transfer the ERC721 to the lender

            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
                _transferERC1155(item, rentalOrder.lender); //@audit-info - transfer the ERC1155
        }

Let's consider the case of ERC721. Here the _transferERC721 function call is as follows:

    function _transferERC721(Item memory item, address recipient) private {
        IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

The safeTransferFrom is called on the ERC721 contract to transfer the token to the lender. Since the ERC721.safeTransferFrom calls the onERC721Received hook on the lender, a malicious lender can revert this transaction by calling revert inside the onERC721Received hook. This will revert the entire Stop.stopRentBatch batch transaction thus not allowing other lenders to receive their respective ERC721 and ERC1155 tokens.

As a result each of the rentalOrders will have to be stopped by calling them individually via the Stop.stopRent function. Hence there will be delay in lenders getting their NFTs back since each order has to be stopped individually. This could incur monetary loss to the lender since he could have used the NFTs for another purpose (such as gaming) if he received the NFT earlier. And this is not good for the user experience as well. The users might lose the trust on the reNFT if they are unable to use the stopRentBatch feature and having to go through the cumbersome process of having to stop one rentalOrder at a time.

Proof of Concept

            _reclaimRentedItems(orders[i]);

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

        for (uint256 i = 0; i < itemCount; ++i) {
            Item memory item = rentalOrder.items[i];

            // Check if the item is an ERC721.
            if (item.itemType == ItemType.ERC721)
                _transferERC721(item, rentalOrder.lender);

            // check if the item is an ERC1155.
            if (item.itemType == ItemType.ERC1155)
                _transferERC1155(item, rentalOrder.lender);
        }

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

    function _transferERC721(Item memory item, address recipient) private {
        IERC721(item.token).safeTransferFrom(address(this), recipient, item.identifier);
    }

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to execute the _reclaimRentedItems(orders[i]) function call inside a try-catch block and handle the single rentalOrder reverts via a fallback appropriately. This will ensure a single malicious lender is unable to deprive other lenders getting their ERC721 and ERC1155 tokens on time and also unable to deprive renters getting their ERC20 tokens (In the event of a PAY order) when the Stop.stopRentBatch is called.

Assessed type

DoS

L-01 Missing salt paramter in EIP 712 eip712DomainTypehash calculation

Lines of code

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

Impact

EIP 712 specifies that a salt param can be used for the domain type has calculation to fully adhere to the specification whereas in this case it is not the case

Tools Used

manual review

Recommended Mitigation Steps

Consider salt when calculating eip712DomainTypehash

L-02 hook item index can be out of bounds with the offer & rental item array

Lines of code

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

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

Impact

There is no check that the item index in the hook array is valid when adding or removing hooks which might result in tx revert when creating/stopping a rental

Tools Used

manual review

Recommended Mitigation Steps

add a validation check to make sure that itemIndex <= offer.length/rental.length

L-03 No check for contract bytecode size during create2 deployment

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Create2Deployer.sol#L54

Impact

deploying a contract without any bytecode will be a waste of execution overhead & gas

Tools Used

manual review

Recommended Mitigation Steps

add a check if(iszero(extcodesize(deploymentAddress))) revert();

L-04 Avoid reacasting the Module instance again

Lines of code

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

Impact

The getModuleForKeycode mapping returns the module instance after casting so we can avoid re-casting to save gas

Tools Used

manual review

Recommended Mitigation Steps

remove re-casting of the module instance

L-05 Use abi.encodePacked for computing eip 712 typehashes

Lines of code

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

Impact

To completely adhere to the EIP 712 standard abi.encodePacked should be used instead of abi.encode when comoputing rentalOrderTypeHash

Tools Used

manual review

Recommended Mitigation Steps

use abi.encodePacked instead of abi.encode when comoputing rentalOrderTypeHash

L-06 Remove extra hook validation check in guard policy

Lines of code

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

Impact

STORE.hookOnTransaction returns whether a hook is valid or not but there is an extra check(hook != address(0)) to determine hook validity when a tx is triggered which is not necessary and costs extra gas

Tools Used

manual review

Recommended Mitigation Steps

update the if statement to if(isActive)