reNFT - zaevlad's results

Lines of code

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

Vulnerability details

Impact

The code has broken EIP-712.

Proof of Concept

Let's take a look at the _deriveTypehashes():

    function _deriveTypehashes()
        internal
        view
        returns (
            bytes32 nameHash,
            bytes32 versionHash,
            bytes32 eip712DomainTypehash,
            bytes32 domainSeparator
        )
    {
        nameHash = keccak256(bytes(_NAME));

        versionHash = keccak256(bytes(_VERSION));

        eip712DomainTypehash = keccak256(
            abi.encodePacked(
                "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
            )
        );

        domainSeparator = _deriveDomainSeparator(
            eip712DomainTypehash,
            nameHash,
            versionHash
        );
    }

There are some problems that violate the EIP-712:

1. EIP712Domain should contian bytes32 salt as a final argument. bytes32 salt an disambiguating salt for the protocol. This can be used as a domain separator of last resort.

2. domainSeparator miss some params and should be cashed in a way:

        domainSeparator = _deriveDomainSeparator(
            eip712DomainTypehash,
            nameHash,
            versionHash,
            block.chainid,
            address(verifyingContract),
            salt
        );

This hashing method is described in https://eips.ethereum.org/EIPS/eip-712#rationale-for-domainseparator

Tools Used

Manual review

Recommended Mitigation Steps

Consider rebuilding the domanSeparator according to the EIP-712 standard

Assessed type

Other