reNFT - rvierdiiev's results

Lines of code

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

Vulnerability details

Proof of Concept

When new order is created, then rent token recipient is checked to be valid safe wallet.

Then later orderHash is created. Pls, note, that safe address is set as rentalWallet in the RentalOrder struct. But during the hashing, rentalWallet is omitted, which means that hash of order doesn't include info about the safe wallet, where tokens are locked.

In order to stop rent, you need to call Stop.stopRent function and provide RentalOrder. It should be exactly the same to provide same hash. As we know rentalWallet is not included into the hash, so attacker can provide any address, but it should be valid safe wallet.

rentalWallet will be used to construct rental assets and later those assets will be removed from debt balance of the safe wallet. The function will first remove orderHash and then will decrease debt of wallet.

The fact that rentalWallet is not included into hash allows attacker to provide any safe wallet that have enough debt of same type and make own orderHash to be deleted.

Example:

• Safe X has borrowed erc1155 in amount of 1000 for 100 days.
• Safe Y has borrowed erc1155 in amount of 500 for 50 days.
• On 51th day, when rent period has finished for Safe Y, attacker(probably owner of Safe Y) calls stopRent for his own order and provides Safe X address as rentalWallet.
• 500 erc1155 tokens then will be sent to the lender of Safe Y rent, order hash for Safe Y rent will be deleted and rentedAssets of erc1155 have decreased for the Safe X wallet to 500.
• On day 101th, someone wants to stop rent for the Safe X, but the call reverts, as it can't decrease rentedAssets for more 1000(and safe wallet doesn't have them).
• Safe Y wallet still have 500 erc1155 tokens. While he can't transfer them out of wallet, he can use them.

Note, this will not work with erc721, only with erc1155 of same id.

Impact

Attacker has ability to steal tokens and make touch assets of other safe wallet.

Tools Used

VsCode

Recommended Mitigation Steps

You need to hash rentalWallet, so only same wallet can be used when stopping rent.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

When new order is created, then lender can provide hooks for the specific offered items. In case if hook can not be used on start, then create is not possible.

This is the first problem and it lies on the fact, that hook can have any amount of 3 bits set that allows it be executed on start, on transaction and on end. So it's really likely that will be a hook, that has nothing to do on start, but has functionality on stop. In that case if such hook for the item will be provided, then rent will not be created.

Then we have another problem, when hook is allowed on start, but is disabled on stop. In this case it will be possible to start rent, but it will be not possible to stop it, which means that settlement will not be done, and tokens will be temporarily stuck.

And similar to the above case is when both start and stop actions for the hook were enabled on order creation and then during the term duration, hook stop action was disabled. Then settlement will revert.

Impact

Hook providing logic is not consistent. In case if hook doesn't have or disabled stop action, then settlement will not occur.

Tools Used

VsCode

Recommended Mitigation Steps

In case if hook is disabled for the action, then just continue, do not revert.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

Signer._deriveRentalTypehashes function creates typehashes for the different functions that can be signed by users of protocol.

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

        bytes memory rentalOrderTypeString = abi.encodePacked(
            "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
        );


        // Derive the Item type hash using the corresponding type string.
        itemTypeHash = keccak256(itemTypeString);


        // Derive the Hook type hash using the corresponding type string.
        hookTypeHash = keccak256(hookTypeString);


        // Derive the RentalOrder type hash using the corresponding type string.
        rentalOrderTypeHash = keccak256(
            abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
        );

This is how rentalOrderTypeHash typehash is created.

From eip:

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

In this example we see, that other structs definition is just added to the root struct definition. But in current implementation we can see that abi.encodePacked is applied for all struct definitions before and only after that they are hashed together.

Exactly in same way rentPayloadTypeHash is constructed.

Impact

Contract doesn't implement eip712 standard which leads to problems with integration and the inability to operate the function to verify the user's signature.

Tools Used

VsCode

Recommended Mitigation Steps

You should not use encoded structs for hashing.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

Signer._deriveRentalTypehashes function creates typehashes for the different functions that can be signed by users of protocol.

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

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


            // Derive RentPayload type hash via combination of relevant type strings.
            rentPayloadTypeHash = keccak256(
                abi.encodePacked(
                    rentPayloadTypeString,
                    orderMetadataTypeString,
                    orderFulfillmentTypeString
                )
            );

This is how rentPayloadTypeHash typehash is created.

From eip:

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

In this example we see, that other structs definitions should be sorted. But in current implementation we can see that OrderMetadata struct is provided before OrderFulfillment struct, which should be not the case.

Impact

Contract doesn't implement eip712 standard which leads to problems with integration and the inability to operate the function to verify the user's signature.

Tools Used

VsCode

Recommended Mitigation Steps

Change the order of structs.

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

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

Create contract follows eip712 standard and implements hashing of structs in the Signer contract.

Signer._deriveOrderMetadataHash function encodes only rent duration and hooks, however its typehash requires more fields.

Same happens to the Signer._deriveRentalOrderHash function that forgets to encode rentalWallet field, however it is declared.

Impact

Contract doesn't implement eip712 standard which leads to problems with integration and the inability to operate the function to verify the user's signature.

Tools Used

VsCode

Recommended Mitigation Steps

Include all needed fields.

Assessed type

Error

Lines of code

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

Vulnerability details

Proof of Concept

Signer._deriveHookHash function provides hash for the hook struct. hook.extraData is bytes array, which should be separately hashed, according to eip712, however function doesn't do that.

From docs: https://eips.ethereum.org/EIPS/eip-712

The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

Impact

Contract doesn't implement eip712 standard which leads to problems with integration and the inability to operate the function to verify the user's signature.

Tools Used

VsCode

Recommended Mitigation Steps

You need to hash hook.extraData separately.

Assessed type

Error