Project
One World
NFTDeFi
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Incorrect setting of `verifyingContract` for Domain Seperator is incompliant with EIP712 and could cause verification to fail

x18a6

Summary

verifyingContract is incorrectly set to EIP712Base instead of NativeMetaTransaction which has NativeMetaTransaction::verify and actually doing the verification. This is incompliant with EIP712 and could cause verification to fail

Vulnerability Details

  • The verifyingContract is supposed to be the address of the contract that will verify the signature.

  • Currently, it is set to address(this) in the EIP712Base contract:

contracts/meta-transaction/EIP712Base.sol#L34

contract EIP712Base {
// ...
function _setDomainSeperator(string memory name, string memory version) internal {
domainSeperator = keccak256(
abi.encode(
EIP712_DOMAIN_TYPEHASH,
keccak256(bytes(name)),
keccak256(bytes(version)),
@>>> address(this),
bytes32(getChainId())
)
);
}
// ...
}

  • EIP712Base::_setDomainSeperator sets the verifyingContract to address(this).

  • This is generally best practice, with a catch: it's under the assumption that the verify function is in the same contract as _setDomainSeperator, making verifyingContract evaluate to the same address doing the verification.

  • However, in the case of this code, the verify function is in the NativeMetaTransaction contract, which inherits from EIP712Base:

contracts/meta-transaction/NativeMetaTransaction.sol#L90-L106

contract NativeMetaTransaction is EIP712Base {
// ...
function verify(
address signer,
MetaTransaction memory metaTx,
bytes32 sigR,
bytes32 sigS,
uint8 sigV
) internal view returns (bool) {
require(signer != address(0), "NativeMetaTransaction: INVALID_SIGNER");
return
signer ==
ecrecover(
toTypedMessageHash(hashMetaTransaction(metaTx)),
sigV,
sigR,
sigS
);
}
// ...
}

To put this all together: The verifyingContract will evaluate to the address of EIP712Base, which is supposed to be the address of the contract that will verify the signature. However, the actual verification will be done by EIP712Base::_setDomainSeperator.

Impact

  • Incompliance with EIP712 as from the specs:

address verifyingContract: the address of the contract that will verify the signature.

Read more in eip-712#definition-of-domainseparator here.

  • In the off-chain code, if verifyingContract is set to NativeMetaTransaction -which is specs compliant since it's the actual contract doing the verification- signatures will mismatch and verification fails since the EIP712 Domain Seperator is constructed using the incorrect EIP712Base as the verifyingContract.

Tools Used

EIP712.

Recommendations

The recommendation depends on the rationale behind seperating EIP712Base and NativeMetaTransaction contracts logic.

  • If it's just to keep the code clean, consider merging the two contracts into one.

  • Otherwise, consider passing the verifyingContract as input to to adderss of EIP712Base::constructor level, then set it to NativeMetaTransaction address.

Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.