0x11singh99 - Wrong `DOMAIN_TYPEHASH` used in calculating `domainSeparator` in `VotingEscrow::delegateBySig`, `EIP712` specification not followed.

[sherlock-admin4]

0x11singh99

Medium

Wrong DOMAIN_TYPEHASH used in calculating domainSeparator in VotingEscrow::delegateBySig, EIP712 specification not followed.

Summary

Wrong DOMAIN_TYPEHASH used in calculating domainSeparator in VotingEscrow::delegateBySig, EIP712 specification not followed.

Vulnerability Details

The delegateBySig function in the VotingEscrow contract computes the domainSeparator using a method that deviates from the EIP-712 standard. Specifically the domainSeparator construction includes a version parameter that is not defined in the DOMAIN_TYPEHASH constant as per EIP-712 specifications. This will cause calculating wrong domainSeparator and finally creating wrong digest. This inconsistency will lead to interoperability issues and could affect the proper validation of signatures across different platforms and tools that rely on EIP-712.

Domain Type Hash (DOMAIN_TYPEHASH):

File : contracts/VotingEscrow.sol

1258: /// @notice The EIP-712 typehash for the contract's domain
1259: bytes32 public constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");

VotingEscrow.sol#L1258-L1259

This defines the structure of the domain type. It includes name, chainId, and verifyingContract.

Domain Separator Calculation:

File : contracts/VotingEscrow.sol

1515: bytes32 domainSeparator = keccak256(
1516:    abi.encode(
1517:        DOMAIN_TYPEHASH,
1518:        keccak256(bytes(name)),
1519:        keccak256(bytes(version)),
1520:        block.chainid,
1521:        address(this)
1522:    )
1523: );

VotingEscrow.sol#L1515-L1523

Here name, version, block.chainid, and address(this) are used to compute the domainSeparator.

The discrepancy lies in the DOMAIN_TYPEHASH definition versus its usage in abi.encode for domainSeparator calculation:

DOMAIN_TYPEHASH includes name, chainId, and verifyingContract. domainSeparator calculation includes name, version, block.chainid, and address(this). According to the EIP-712 standard. the DOMAIN_TYPEHASH should strictly define the type structure expected by the signing process.

Impact

version is included in the domainSeparator calculation but not in the DOMAIN_TYPEHASH. This deviation from the standard could lead to interoperability issues and confusion, especially when verifying signatures across different implementations or tools that strictly adhere to EIP-712.

Interoperability Issues: Signatures generated using the current method may not be verifiable by systems strictly adhering to the EIP-712 standard potentially leading to signature validation failures.

Security Risk: Misaligned or non-standard implementation of signature schemes can introduce confusion and increase the risk of vulnerabilities such as signature replay attacks.

Tools Used

Manual Review

Recommendations

To adhere to the EIP-712 standard:

Modify DOMAIN_TYPEHASH to include version.

- bytes32 public constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,uint256 chainId,address verifyingContract)");
+ bytes32 public constant DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

By aligning DOMAIN_TYPEHASH with the actual data used to compute domainSeparator you ensure compatibility with tools and libraries that conform to the EIP-712 standard. This alignment is crucial for correctly interpreting and verifying signatures generated using EIP-712.

Recommendation

Duplicate of #126