search

brinktrade / brink-core

Core smart contracts for Brink accounts
GNU General Public License v3.0
12 stars 7 forks source link

Audit Issue 8: Removing immutable chain id #20

Closed Kyrrui closed 2 years ago

Kyrrui commented 2 years ago

WARNING: Some tests fail, needs to be looked at

NOTE: If we upgrade to solidity 0.8.0 we can get some gas savings here by using block.chainId

mikec commented 2 years ago

@Kyrrui I disagree with the audit suggestion here, I don't think we should use the actual chainId value. The value of chainId that we're using is actually arbitrary, it doesn't have to match the real chainId. It's really just used to make sure that a message signed for one chain cannot be replayed on another. It's up to us to deploy to each chain with a different chainId.

We're using the same pattern that DAI uses for EIP712 permit. See https://etherscan.io/address/0x6b175474e89094c44da98b954eedeac495271d0f#code#L110

mikec commented 2 years ago

also if we used the actual chainId value here, and the chainId was changed by a hardfork, it would invalidate all existing signed messages