Submission Details
Severity:
Contract will not be able to verify Signatures after HardFork.
Contract will not be able to verify Signatures after HardFork.
Brief
In an event Of a Hard Fork, the Chain Id changes, therby changing return vaulue of getDomainSeperator(). This will hence result in contract calculating a wrong digest for attempted ECDSA Verification.
Hence a Permanent DOS to all metaTransactions!
The EIP712Base.sol implementation was too optimistic and doesnt account for the fact that there could be a change in chainId value.
Impact
In an event of a HardFork, contract will no longer be able to verify signed tx. Hence a Permanent DOS to MetaTransactions.
Severity
Impact: High
Possibility: Medium
Notice
Important to distinguish between this issue and the issue of "Not using toTypedMessageHash()".
Simply put, even if toTypedMessageHash() were used correctly, this bug still persists(Verification will fail after HardFork).
Its root cause is unique.
POC
Note: The ommited toTypedMessageHash() was included here to showcase this:
// SPDX-License-Identifier: UNLICENSED
import {Test, console} from "forge-std/Test.sol";
import {MembershipFactory} from "../src/dao/MembershipFactory.sol";
//import {IMembershipFactory} from "./IMembe"
import {NativeMetaTransaction, IMembershipFactory} from "./IMembershipFactory.sol";
import {CurrencyManager} from "../src/dao/CurrencyManager.sol";
import {MembershipERC1155} from "../src/dao/tokens/MembershipERC1155.sol";
contract MembershipFactoryTest is Test {
uint256 depositorPvKey;
address depositor;
MembershipERC1155 erc1155;
CurrencyManager currencyMgr;
IMembershipFactory factory;
string baseURI = "https://baseuri.com/";
address OpWallet = makeAddr("OpWallet");
// Alterable Variables @ HardFork
uint256 constant POST_HARD_FORK_CHAIN_ID = 33333;
bool hardForked;
//Constank variables even After HardFork
bytes32 internal constant EIP712_DOMAIN_TYPEHASH =
keccak256(bytes("EIP712Domain(string name,string version,address verifyingContract,bytes32 salt)"));
function setUp() public {
(depositor, depositorPvKey) = makeAddrAndKey("Depositor");
erc1155 = new MembershipERC1155();
currencyMgr = new CurrencyManager();
factory = IMembershipFactory(
address(new MembershipFactory(address(currencyMgr), OpWallet, baseURI, address(erc1155)))
);
}
function testContractUnableToVerifyAfterHardFork() public {
//==================signing befor HardFork================================
//Variables
uint256 nonce = factory.getNonce(depositor);
bytes memory FunctionSig = abi.encodeCall(IMembershipFactory.baseURI, ()); //Calling a Random Function
bytes32 r;
bytes32 s;
uint8 v;
bytes32 digest = computeDigest(nonce, depositor, FunctionSig);
console.log("digest");
console.logBytes32(digest);
(v, r, s) = vm.sign(depositorPvKey, digest);
assertEq(depositor, ecrecover(digest, v, r, s));
//Relayer Executes
bytes memory retData = factory.executeMetaTransaction(depositor, FunctionSig, r, s, v);
string memory returnedBaseURI = abi.decode(retData,(string));
assertEq(keccak256(abi.encode(returnedBaseURI)),keccak256(abi.encode(factory.baseURI())) );
//=======================================================================//
//==========================AFTER HARDFork===============================//
HARD_FORK();
//==================signing After HardFork================================
uint256 nonce2 = factory.getNonce(depositor);
//bytes memory FunctionSig = abi.encodeCall(IMembershipFactory.baseURI, ()); //Calling a Random Function
bytes32 r2;
bytes32 s2;
uint8 v2;
bytes32 digest2 = computeDigest(nonce, depositor, FunctionSig);
console.log("digest2");
console.logBytes32(digest2);
(v2, r2, s2) = vm.sign(depositorPvKey, digest2);
//Confirms signature was by inputed address ie depositor
assertEq(depositor, ecrecover(digest2, v2, r2, s2));
//Relayer Executes
vm.expectRevert();
factory.executeMetaTransaction(depositor, FunctionSig, r2, s2, v2);
}
function getChainId(bool _postHardFork) private view returns (uint256 chainId) {
if (_postHardFork) {
chainId = POST_HARD_FORK_CHAIN_ID;
console.log(_postHardFork);
console.log("chainId",chainId);
} else {
chainId = factory.getChainId();
console.log(_postHardFork);
console.log("chainId",chainId);
}
}
function computeDigest(uint256 _nonce, address _signer, bytes memory _funcSig) internal returns (bytes32 digest) {
return toTypedMessageHash(
factory.hashMetaTransaction(NativeMetaTransaction.MetaTransaction(_nonce, _signer, _funcSig))
);
}
function HARD_FORK() private {
hardForked = true;
console.log("==================================CHAINFORKED()=======================");
console.log("==================================CHAINFORKED()=======================");
console.log("==================================CHAINFORKED()=======================");
console.log("==================================CHAINFORKED()=======================");
}
function toTypedMessageHash(bytes32 messageHash) private returns (bytes32) {
return keccak256(abi.encodePacked("\x19\x01", getDomainSeperator(), messageHash));
}
function getDomainSeperator() private returns(bytes32){
return keccak256(
abi.encode(
EIP712_DOMAIN_TYPEHASH,// CoNSTANT (Does not change after HardFork)
keccak256(bytes("OWP")), // CoNSTANT (Does not change after HardFork)
keccak256(bytes("1")), // CoNSTANT (Does not change after HardFork)
address(factory),// CoNSTANT (Address Does not change after HardFork)
getChainId(hardForked) // ++++++++++++++MUTATE+++++++WILL CHANGE @HARDFORK+++++++++++++++++++++++++
)
);
}
}
Mitigation
Openzeppelin Caches ChainId at construction time and when getDomainSeperator() is called, we check if cached chainId has changed. If changed, then new domainSeparator is computed.
Something like this:
diff --git a/EIP712Base.sol b/EIP712Base.sol
index 64db0ec..927f5a4 100644
--- a/EIP712Base.sol
+++ b/EIP712Base.sol
contract EIP712Base {
)
);
bytes32 internal domainSeperator;
+ bytes32 internal cachedChainID;
+ string public name_;
+ string public version_;
constructor(
string memory name,
string memory version
){
+ name_ = name;
+ version_ = version;
_setDomainSeperator(name, version);
}
function _setDomainSeperator(string memory name, string memory version) internal {
+ cachedChainID =bytes32(getChainId());
domainSeperator = keccak256(
abi.encode(
EIP712_DOMAIN_TYPEHASH,
keccak256(bytes(name)),
keccak256(bytes(version)),
address(this),
- bytes32(getChainId())
+ cachedChainID
)
);
}
- function getDomainSeperator() public view returns (bytes32) {
- return domainSeperator;
+ function getDomainSeperator() public returns (bytes32) {
+ if(cachedChainID == bytes32(getChainId())){
+ return domainSeperator;
+ }else{
+ _setDomainSeperator(name_,version_);
+ return domainSeperator;
+
+ }
+
}
function getChainId() public view returns (uint256) {
contract EIP712Base {
*/
function toTypedMessageHash(bytes32 messageHash)
internal
- view
returns (bytes32)
{
return
(END)
Updates
Prize pool breakdown
Total prize
15,000 USDC
nSLOC
54128 USDC / LOC
14,000 USDC
1,000 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.