Protocol does not implement EIP712 correctly on multiple occasions

[c4-bot-6]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L151-L151 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#L384-L386 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L232-L238 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Create.sol#L636

Vulnerability details

Impact

Being not EIP712 compliant can lead to issues with integrators and possibly DOS.

Problem 1

The implementation of the hook hash (here) is done incorrectly. hook.extraData is of type bytes which according to EIP712 it is referred to as a dynamic type. Dynamic types must be first hashed with keccak256 to become one 32-byte word before being encoded and hashed together with the typeHash and the other values.

Mitigation to Problem 1:

function _deriveHookHash(Hook memory hook) internal view returns (bytes32) {
  // Derive and return the hook as specified by EIP-712.
    return
        keccak256(
-           abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, hook.extraData)
+           abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, keccak256(hook.extraData))
        );
}

Problem 2

Some TypeHashes are computed by using abi.encode instead of abi.encodePacked (here) which makes the typeHash value and the whole final hash different from the hash that correctly implementing EIP712 entities would get.

// Construct the Item type string.
bytes memory itemTypeString = abi.encodePacked(
    "Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
);

// Construct the Hook type string.
bytes memory hookTypeString = abi.encodePacked(
    "Hook(address target,uint256 itemIndex,bytes extraData)"
);

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

...

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

The problem with this is that abi.encode-ing strings results in bytes with arbitrary length. In such cases (like the one here) there is a high chance that the bytes will not represent an exact N words in length (X * 32 bytes length) and the data is padded to conform uniformly to 32-byte words. This padding results in an incorrect hash of the typeHash and it will make the digest hash invalid when compared with properly implemented hashes from widely used libraries such as ethers.

Proof of Concept

Place the following code in any of the tests and run forge test -—mt test_EIP712_encoding

function test_EIP712_encoding() public {
        // Copied from the reNFT codebase
    bytes memory itemTypeString = abi.encodePacked(
        "Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
    );
    bytes memory hookTypeString = abi.encodePacked(
        "Hook(address target,uint256 itemIndex,bytes extraData)"
    );
    bytes memory rentalOrderTypeString = abi.encodePacked(
        "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)"
    );
        // protocol implementation
    bytes32 rentalOrderTypeHash = keccak256(
        abi.encode(rentalOrderTypeString, hookTypeString, itemTypeString) // <-----
    );

    // correct implementation
    bytes32 rentalOrderTypeHashCorrect = keccak256(
        abi.encodePacked(rentalOrderTypeString, hookTypeString, itemTypeString) // <-----
    );

    // the correct typehash
    bytes32 correctTypeHash = keccak256(
        "RentalOrder(bytes32 seaportOrderHash,Item[] items,Hook[] hooks,uint8 orderType,address lender,address renter,address rentalWallet,uint256 startTimestamp,uint256 endTimestamp)Hook(address target,uint256 itemIndex,bytes extraData)Item(uint8 itemType,uint8 settleTo,address token,uint256 amount,uint256 identifier)"
    );

    assertNotEq(rentalOrderTypeHash, rentalOrderTypeHashCorrect);
    assertNotEq(rentalOrderTypeHash, correctTypeHash);
    assertEq(rentalOrderTypeHashCorrect, correctTypeHash);
}

This test shows that the rentalOrderTypeHashCorrect is the correct typeHash.

Mitigation to Problem 2

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

Problem 3

_deriveOrderMetadataHash constructs the hash incorrectly because _ORDER_METADATA_TYPEHASH includes uint8 orderType and bytes emittedExtraData (it can be seen here) but these values are not provided below the typeHash (it can be seen here).

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) {
        hookHashes[i] = _deriveHookHash(metadata.hooks[i]);
    }

    return
        keccak256(
            abi.encode(
                _ORDER_METADATA_TYPEHASH,// OrderMetadata(uint8 orderType,uint256 rentDuration,Hook[] hooks,bytes emittedExtraData)
                                // <---- misses uint8 orderType
                metadata.rentDuration,
                keccak256(abi.encodePacked(hookHashes))
                                // <---- misses bytes emittedExtraData
            )
        );
}

This hash is important because it is compared to the zoneHash inside Create.sol:validateOrder → _rentFromZone → _isValidOrderMetadata (link): So if the provided zoneHash from Seaport was generated correctly and it is not the same as the one generated by reNFT, the protocol will not be able to create any rentalOrders resulting in DOS.

In any case, this implementation is not according to EIP712 and either the fields must be included or the _ORDER_METADATA_TYPEHASH must remove uint8 orderType and bytes emittedExtraData

Tools Used

Manual review and Foundry for testing

Recommendations

Apply the described fixes for each example

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

Alec1017 (sponsor) confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean removed the grade

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report