safe-global / safe-smart-account

Safe allows secure management of blockchain assets.
https://safe.global
GNU Lesser General Public License v3.0
1.84k stars 907 forks source link

Mask Immutables in Local Verify Task #691

Closed nlordell closed 11 months ago

nlordell commented 11 months ago

Fixes #689

The local verification task does not work correctly for contracts with immutables. This is because, immutable values get written when the contract's init code executes on-chain, but uses 0 place holders in the deployedBytecode compiler output.

This causes the code that is fetched from the deployed contract to differ slightly from the compiler's byte code output (fetched code has non-0 immutable values, while the compiler output has 0 immutable values).

The fix is to mask the immutables from the fetched code with 0's before comparing.

With this fix, local-verify script now works for the SimulateTxAccessor contract:

$ npx hardhat local-verify --network localhost
Verification status for CompatibilityFallbackHandler: SUCCESS
Verification status for CreateCall: SUCCESS
Verification status for MultiSend: SUCCESS
Verification status for MultiSendCallOnly: SUCCESS
Verification status for Safe: SUCCESS
Verification status for SafeL2: SUCCESS
Verification status for SafeProxyFactory: SUCCESS
Verification status for SignMessageLib: SUCCESS
Verification status for SimulateTxAccessor: SUCCESS
Verification status for TokenCallbackHandler: SUCCESS
github-actions[bot] commented 11 months ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

nlordell commented 11 months ago

I have read the CLA Document and I hereby sign the CLA

nlordell commented 11 months ago

I looked into it, and the constructor arguments are not included in the Hardhat deployment data.

mmv08 commented 11 months ago

I looked into it, and the constructor arguments are not included in the Hardhat deployment data.

We don't have any arguments, do we? There is an "args" field in the artifacts with an empty array.

mmv08 commented 11 months ago

Also I found this: https://ethereum.stackexchange.com/questions/94396/how-to-get-the-actual-runtime-bytecode-from-creation-bytecode-and-constructor-ar

nlordell commented 11 months ago

Also I found this: https://ethereum.stackexchange.com/questions/94396/how-to-get-the-actual-runtime-bytecode-from-creation-bytecode-and-constructor-ar

You can also do an eth_call to do the comparison. For example, the following command will show no diff:

diff -u --color \
    <(cat deployments/localhost/SafeProxyFactory.json | jq '.deployedBytecode') \
    <(curl -s "$NODE_URL" -X POST -H 'content-type: application/json' --data "{\"jsonrpc\":\"2.0\",\"method\":\"eth_call\",\"id\":0,\"params\":[{\"data\":$(cat deployments/localhost/SafeProxyFactory.json | jq '.bytecode')}]}" | jq '.result')
nlordell commented 11 months ago

We don't have any arguments, do we?

🤦 - correct. For some reason I assumed that the ACCESSOR_SINGLETON immutable was set from a constructor argument, but it is just read from the transaction context. The difficulty here is that the value for this that is used to set the immutable value is not easy to fudge.

mmv08 commented 11 months ago

We don't have any arguments, do we?

🤦 - correct. For some reason I assumed that the ACCESSOR_SINGLETON immutable was set from a constructor argument, but it is just read from the transaction context. The difficulty here is that the value for this that is used to set the immutable value is not easy to fudge.

Then I'd say it's good as is