Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
GalloDaSballo opened this issue Nov 28, 2024 · 8 comments

Comments

@GalloDaSballo
Copy link

GalloDaSballo commented Nov 28, 2024

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

@GalloDaSballo GalloDaSballo changed the title Echidna StartPrank overrides msg.sender at all levels, making it inconsistent vs foundry and causing unintended operations hevm startPrank overrides msg.sender at all call depth levels, making it inconsistent vs foundry and causing unintended operations Nov 28, 2024
@msooseth
Copy link
Collaborator

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
Copy link
Collaborator

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

@elopez
Copy link
Collaborator

elopez commented Nov 28, 2024

@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
Copy link
Collaborator

elopez commented Nov 28, 2024

@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: crytic/echidna#1331

@GalloDaSballo
Copy link
Author

Thank you all, will test it soon

@msooseth
Copy link
Collaborator

msooseth commented Nov 28, 2024

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

msooseth/echidna#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.

msooseth added a commit that referenced this issue Nov 28, 2024
Related to #608
@GalloDaSballo
Copy link
Author

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

@msooseth
Copy link
Collaborator

msooseth commented Dec 2, 2024

Closing as it's now passing and merged. Thanks a lot! :)

@msooseth msooseth closed this as completed Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants