reNFT - Ward's results

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L183-L193 https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/packages/Signer.sol#L233-L237

Vulnerability details

Impact

The _deriveRentalOrderHash and _deriveOrderMetadataHash functions in the Signer.sol contract lack the utilization of all parameters from the structs intended for hash derivation according to the EIP-712 specifications. This omission could lead to identical hashes if it becomes possible to input the same values for all parameters except the currently excluded ones. This practice of excluding some parameters not only diminishes uniqueness and expands the potential attack surface it also introduces incompatibility with the EIP-712 specification.

Also note that unlike _deriveRentalOrderHash and _deriveOrderMetadataHash, the typeHashes of OrderMetada and RentalOrder are derived correctly, with all parameters included.

Proof of Concept

Parameters used in _deriveRentalOrderHash, rentalWallet is missing.

// Signer.sol::_deriveRentalOrderHash
keccak256(
    abi.encode(
        _RENTAL_ORDER_TYPEHASH,
        order.seaportOrderHash,
        keccak256(abi.encodePacked(itemHashes)),
        keccak256(abi.encodePacked(hookHashes)),
        order.orderType,
        order.lender,
        order.renter,
        order.startTimestamp,
        order.endTimestamp
    )
);

// RentalStructs.sol
struct RentalOrder {
    bytes32 seaportOrderHash;
    Item[] items;
    Hook[] hooks;
    OrderType orderType;
    address lender;
    address renter;
    address rentalWallet;
    uint256 startTimestamp;
    uint256 endTimestamp;
}

Parameters used in _deriveOrderMetadataHash, orderType and emittedExtraData is missing.

// Signer.sol::_deriveOrderMetadataHash
keccak256(
    abi.encode(
        _ORDER_METADATA_TYPEHASH,
        metadata.rentDuration,
        keccak256(abi.encodePacked(hookHashes))
    )
);

// RentalStructs.sol
struct OrderMetadata {
    // Type of order being created.
    OrderType orderType;
    // Duration of the rental in seconds.
    uint256 rentDuration;
    // Hooks that will act as middleware for the items in the order.
    Hook[] hooks;
    // Any extra data to be emitted upon order fulfillment.
    bytes emittedExtraData;
}

Tools Used

Manual Review

Recommended Mitigation Steps

In the hash derivation functions in question, add all the parameters mentioned in the structs, taking into account the EIP-712 specification.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The hook.extraData parameter which is being included at the hash of the hook, is a bytes type, which EIP-712 defines as dynamic. Dynamic types are encoded as the hash of the contents; and currently the extraData parameter is being encoded as-is.

That could result in unexpected integration failures with EIP712-compliant wallets or tooling that perform the encoding in the appropriate way.

Proof of Concept

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

// Signer.sol::_deriveHookHash
    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)
            );
    }

Hook struct defined in RentalStructs.sol, extraData is bytes type

struct Hook {
    // The hook contract.
    address target;
    // Index of the item in the order to apply the hook to.
    uint256 itemIndex;
    // Any extra data that the hook will need.
    bytes extraData;
}

Tools Used

Manual Review

Recommended Mitigation Steps

Encode the dynamic hook.extraData parameter as per the EIP-712 specification.

return keccak256(abi.encode(_HOOK_TYPEHASH, hook.target, hook.itemIndex, keccak256(hook.extraData)));

Assessed type

Other