ethereum / sourcify

Decentralized Solidity contract source code verification service
https://sourcify.dev
MIT License
782 stars 398 forks source link

Reinit Issue #1067

Open Hellobloc opened 1 year ago

Hellobloc commented 1 year ago

Introduction

Reinit problem is the deployment of two different runtime codes on the same address via create2. This can lead to a mismatch in our source code, and we found that Sourcify source code results do not warn about reinit, which causes the source code mismatch to go unnoticed by the public.

Impact

An attacker can use this feature to hide a backdoor in the source code and defraud the public.

Attack Case

https://repo.sourcify.dev/contracts/partial_match/420/0x341577fB771EBFB4FaF74fBcF786d4F7Ce02BBaB/sources/ https://goerli.optimism.blockscout.com/address/0x341577fB771EBFB4FaF74fBcF786d4F7Ce02BBaB

image

image

View in Huly HI-473

kuzdogan commented 1 year ago

Hey thanks for the report. I just don't understand how in this contract it is possible to change the string to "evil return" when it's being redeployed. Doesn't the init code hash has to change, so that the address will also change?

sealer3 commented 1 year ago

The issue is that CREATE2 only ensures that the address is always given the same initcode. The issue as @Hellobloc pointed out here is that the same initcode can produce arbitrary bytecode. For example, there might be a branch in the initcode that deploys the "evil return" bytecode after a certain timestamp or first reads some other contract to get the bytecode that should be written.

Example repo that accomplishes same-address CREATE2 upgrades: https://github.com/ethereumgb/create2-upgrade Blog post by ricmoo (ethers.js creator) about this: https://blog.ricmoo.com/wisps-the-magical-world-of-create2-5c2177027604

I think this should no longer be an issue after the selfdestruct opcode is changed with EIP-6780, but watch out on other chains.

kuzdogan commented 1 year ago

Now I understand better, thanks a lot for the explanation and the examples @sealer3

One thing to consider thinking about this is Sourcify is essentially not an interface, and not a block explorer, unlike Etherscan. Our repo.sourcify.dev is only meant to be a way to navigate files in the repo. A better way to think about seeing Sourcified files is a block explorer like Otterscan (choose "Sourcify servers" from top right menu) or like these Avalanche Subnet explorers.

Etherscan can warn the users on their interface with a "Reinit" warning because:

  1. They actively monitor chains and can mark any self-destructed contract. We don't.
  2. They can show on their UI as it's the primary way to see source code BUT apparently they don't return any flags or additional info about the Reinit through the API

This attack would not be possible with our CREATE2 verification because our CREATE2 assumes the initcode === creation bytecode of the contract that will be created with CREATE2.

https://github.com/ethereum/sourcify/blob/941886b187301a0c7aea55b02c19e4f39440f550/packages/lib-sourcify/src/lib/verification.ts#L226-L231

However, this is still an issue with a "normal" verification and I think this is the same issue as https://github.com/ethereum/sourcify/issues/933. SELFDESTRUCT is opening the gates for all kinds of problems that can only be solved with active monitoring of the chain if the SELFDESTRUCT is called on contract or not... I am not really sure how to workaround this problem.

On the positive side, if a user is to do what we call "edge-verification" i.e. pulling files from IPFS and verifying locally on their machine, then it will not be possible to fool the user, as at the time of verification the runtime bytecode would be different, and the verification will fail.

sealer3 commented 1 year ago

Is the bytecode or bytecode hash recorded anywhere in the Sourcify database that would make it easier for explorers like Otterscan to check that the bytecode is the same without having to recompile the contract?

Edit: I think the whole point of edge verification is only having to trust the links to the compiler instead of the database, so to answer my own question, maybe downstream explorers can just check the bytecode themselves if necessary.

Hellobloc commented 1 year ago

Many thanks to @sealer3 for the specific description of these issues. just like @sealer3 mentioned here,blockscout also mentions the need for verified bytecode in this issue

Hellobloc commented 1 year ago

@kuzdogan Awesome research, we also noticed that etherscan does not show any error report in APIs just like you mentioned above and we already reported it to Etherscan, but they have not replied yet. o(╥﹏╥)o

Anyway, thank you very much for your attention to these issues, I see that other explorers are also discussing these kinds of issues responsibly. So I would love for the following suggestion to become a reality. I'm curious if sourcify can build a group to enable multiple explorers to initiate risk discussions related to source code verification, such as the reinit issue and the linked library issue, which have been around for a long time in ethereum.

kuzdogan commented 1 year ago

Is the bytecode or bytecode hash recorded anywhere in the Sourcify database that would make it easier for explorers like Otterscan to check that the bytecode is the same without having to recompile the contract?

Edit: I think the whole point of edge verification is only having to trust the links to the compiler instead of the database, so to answer my own question, maybe downstream explorers can just check the bytecode themselves if necessary.

@sealer3 No, we don't store the bytecode or its hash. In fact Sourcify is just a filesystem without a DB, as the main goal is to share the files over IPFS. For now, it's been working fine but we wanted to explore if we can retain the IPFS pinning in a DB through IPLD etc. If you know anything, please lmk.

I'm curious if sourcify can build a group to enable multiple explorers to initiate risk discussions related to source code verification, such as the reinit issue and the https://github.com/blockscout/blockscout-rs/issues/532, which have been around for a long time in ethereum.

@Hellobloc Sure we are already in contact with Blockscout. But also, it's better to keep the discussion here and open as much as we can, when it's not strictly required to be private as they are security issues.

marcocastignoli commented 1 year ago

We could detect the selfdestruct opcode in the bytecode (if there is a way) and save a file in the sourcify repo SELFDESTRUCT to flag the contract as potentially vulnerable.

Hellobloc commented 1 year ago

@marcocastignoli Checking the selfdestruct in the bytecode may not be useful, as the linked library can be used to self-destruct。

marcocastignoli commented 1 year ago

What about checking also the linked libs? :D

Hellobloc commented 1 year ago

Is it okay to store the bytecode and feed it back to user, and then get the runtime code to check when someone requests the source code of a contract? This means that

  1. for browsers, they should also store your runtime code during request, and they should be responsible for marking reinit risks when the runtime code changes.
  2. for normal users, they just get the contract source code in a single request and sourcify can check the runtime code and give a risk alert when they request it. Also, you can delete or mark these reinit contracts.
Hellobloc commented 1 year ago

@marcocastignoli Checking linked libraries is too complicated, and there may be recursive calls, which may introduce other risks, such as two linked libraries calling each other, which would be a dead loop. But checking selfdestruct is a wonderful thing for users, and if Tonado cash had such a check, maybe it wouldn't have sent that attack.

marcocastignoli commented 1 year ago

I think we can handle the dead loop by simply not reiterating if we encounter the same bytecode two times with the same address. Does this make sense?

Also why you say checking linked libraries is too complicated?