ethereum / hevm

symbolic EVM evaluator
https://hevm.dev
GNU Affero General Public License v3.0
241 stars 48 forks source link

hevm `startPrank` overrides msg.sender at all call depth levels, making it inconsistent vs foundry and causing unintended operations #608

Open GalloDaSballo opened 4 days ago

GalloDaSballo commented 4 days ago

Impact

I have written this repo which shows a full repro

https://github.com/Recon-Fuzz/prank-echidna-vs-foundry

Fundamentally, when using startPrank and calling a contract that performs a transferFrom, echidna (using hevm under the hood), will perform the transferFrom with the pranked address instead of the router, causing the transfer to fail

Mitigation

I believe that the prank should only be enforced at the current call level, a nested call should not longer have it's msg.sender altered

msooseth commented 4 days ago

Right, so overrideCaller is set from Nothing to the address when startPrank is called. This also sets resetCaller to False. Then, when stopPrank is called, overrideCaller is set to Nothing, and resetCaler is set to True.

So what you are saying would I think mean that when we do another call, overrideCaller should be reset to Nothing, along with resetCaller being reset to True. Let me make a PR like that, and maybe you can then check if that fixes it. Would that be OK?

msooseth commented 3 days ago

So I have a new branch, fix_prank that may fix this. I'm trying to check if it does now :)

elopez commented 3 days ago

@msooseth I don't think that would be enough, as when you return from the call, overrideCaller should be set back to the address and resetCaller should be back to True. I'm thinking those two options should be moved to the frame rather than being a global VM setting.

EDIT: I think you did just that on the branch, it'll probably work fine now 💯 thanks!

From what I understand it should be like this:

A.foo()
\__startPrank(X)
|__B.bar() /* msg.sender is X */
|    \__C.baz() /* msg.sender is B, as the override only applies on A.foo's frame */
|__B.baz() /* msg.sender is X */
|__stopPrank()
|__B.bar() /* msg.sender is A */
     \__C.baz() /* msg.sender is B */
elopez commented 3 days ago

@GalloDaSballo I opened a draft PR on Echidna that uses @msooseth's hevm branch, if you can try it out and see if it behaves as expected now, that'd be great: https://github.com/crytic/echidna/pull/1331

GalloDaSballo commented 3 days ago

Thank you all, will test it soon

msooseth commented 3 days ago

OK, seems to be fixed with c6c4b44af0137ccfdab8197a7dd1e4ae69114edb. The corresponding echidna is:

https://github.com/msooseth/echidna/pull/1

You can cone that, go into fix_prank branch, then nix shell, then git clone https://github.com/Recon-Fuzz/prank-echidna-vs-foundry then cd prank-echidna-vs-foundry and then echidna . --contract CryticTester --config echidna.yaml and it should all pass.

GalloDaSballo commented 2 days ago

Have tested this release candidate by @elopez : https://github.com/crytic/echidna/actions/runs/12081766889

I ran it for 1 Million tests with no reverts

Screenshot 2024-11-29 at 18 05 39

In contrast see the previous version failing in less than 50k tests

Screenshot 2024-11-29 at 17 59 54