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

Mutable domain separator in `EIP712Base::_setDomainSeperator` invalidate previously signed transactions, cause replay issues

safdie

Summary

The EIP712Base contract contains a mutable domain separator that could inadvertently be modified by inheriting contracts. Although the _setDomainSeperator function is internal and not directly accessible externally, exposing it in a derived contract or allowing it to be called more than once could result in the domain separator being altered post-deployment. This change could invalidate previously signed transactions, cause replay issues, and expose users to unexpected behaviors.

Vulnerability Details

The domain separator in the EIP-712 standard is intended to provide a unique identity for the contract and domain-specific data to avoid cross-chain or cross-platform replay attacks. In the EIP712Base contract, the domainSeperator is set during construction via _setDomainSeperator but is not protected from being called again, allowing potential modification if exposed by derived contracts.

https://github.com/Cyfrin/2024-11-one-world/blob/1e872c7ab393c380010a507398d4b4caca1ae32b/contracts/meta-transaction/EIP712Base.sol#L28-L38

contract EIP712Base {
// ...
bytes32 internal domainSeperator;
​
constructor(string memory name, string memory version) {
_setDomainSeperator(name, version);
}
​
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())
)
);
}
​
// Other functions
}

Since _setDomainSeperator is an internal function, any derived contract could call it to update domainSeperator. If domainSeperator is modified, any previously signed transactions with the old separator would become invalid. Furthermore, replay protection relies on a stable domain separator, and any unintended alteration could expose users to replay attacks or prevent them from executing expected transactions.

PoC:

  • Let's define a derived contract that exposes _setDomainSeperator, allowing us to simulate a domain separator change.

// Expose _setDomainSeperator function for testing purposes
contract ExposedEIP712Base is EIP712Base {
constructor() EIP712Base("TestName", "1") {}
​
function changeDomainSeparator(string memory newName, string memory newVersion) public {
_setDomainSeperator(newName, newVersion);
}
}

  • Using Foundry, we can deploy ExposedEIP712Base and verify the domainSeperator before and after calling changeDomainSeparator.

function test_DomainSeparatorChange() public {
ExposedEIP712Base contractInstance = new ExposedEIP712Base();
​
// Check initial domain separator
bytes32 initialDomainSeparator = contractInstance.getDomainSeperator();
​
// Change domain separator
contractInstance.changeDomainSeparator("NewName", "2");
​
// Verify domain separator has changed
bytes32 newDomainSeparator = contractInstance.getDomainSeperator();
assertTrue(initialDomainSeparator != newDomainSeparator, "Domain separator should have changed");
}

  • The test shows that the initial domain separator differs from the new domain separator after calling changeDomainSeparator, demonstrating the vulnerability.

Impact

  1. Any user signatures that were signed with the old domain separator become invalid.

  2. Modifying the domain separator can undermine the EIP-712's primary goal of protecting against cross-domain replay attacks, exposing users to potential unauthorized replay scenarios.

  3. If the domain separator changes unexpectedly, users may encounter failed transactions, causing frustration and potentially leading to a loss of trust in the contract.

Tools Used

Manual review.

Recommendations

  1. Define domainSeperator as immutable in the constructor, so it cannot be changed after initialization.

  2. Set _setDomainSeperator to be called only once by using an initializer pattern if deploying via proxies, or structuring it to prevent multiple calls.

contract EIP712Base {
bytes32 internal immutable domainSeperator;
​
constructor(string memory name, string memory version) {
domainSeperator = keccak256(
abi.encode(
EIP712_DOMAIN_TYPEHASH,
keccak256(bytes(name)),
keccak256(bytes(version)),
address(this),
bytes32(getChainId())
)
);
}
}
Updates

Lead Judging Commences

0xbrivan2 Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
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.

Give us feedback!