EIP-712 typehash is incorrect for `RentalOrder` and `RentPayload` structs

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

According to the EIP-712 standard, if a struct type references other struct types, the set of referenced struct types should be appended directly to the encoding of the type containing them, and this set should be sorted by name (1). However, in the current implementation in the Signer contract, the _RENTAL_ORDER_TYPEHASH and _RENT_PAYLOAD_TYPEHASH are not derived correctly, leading to non-compliance with the EIP-712 standard.

Proof of Concept

The rentalOrderTypeHash returned by the _deriveRentalTypehashes() function, which is stored as _RENTAL_ORDER_TYPEHASH in the constructor, is meant to represent the EIP-712 typehash for the RentalOrderStruct. However, it is computed as: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L373-L375

rentalOrderTypeHash = keccak256(
    abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString)
);

This is incorrect, as EIP-712 specifies that the set of referenced struct types is appended directly to the encoding of the type containing them, which can be achieved by using abi.encodePacked():

rentalOrderTypeHash = keccak256(
    abi.encodePacked(rentalOrderTypeString, hookTypeString, itemTypeString)
);

This is equivalent to keccak256(string.concat(rentalOrderTypeString, hookTypeString, itemTypeString)) and EIP-712 compliant.

Furthermore, the typehash for the RentPayload struct is derived correctly using abi.encodePacked(), but the correct sorting order is not observed as the OrderMetadata type string is appended before the OrderFulfillment type string: https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L394-L400

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

This breaks EIP-712 compliance, so can be seen as an instance of “function of the protocol or its availability could be impacted” and is hence classified as medium severity.

Tools Used

Manual review

Recommended Mitigation Steps

To fix these issues, the _deriveRentalTypehashes() function in Signer.sol should be updated to correctly derive the rentalOrderTypeHash and rentPayloadTypeHash according to the EIP-712 standard.

1. For rentalOrderTypeHash, use abi.encodePacked(rentalOrderTypeString, hookTypeString, itemTypeString) instead of abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString).
2. For rentPayloadTypeHash, sort orderMetadataTypeString and orderFulfillmentTypeString by name before appending them to the encoding.

This will ensure that the derived type hashes are compliant with EIP-712 and function correctly in the protocol and with future external integrations.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #239

[c4-judge]

0xean marked the issue as satisfactory