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

EVM: respect prank address in callChecks #379

Merged
merged 5 commits into from
Nov 22, 2023
Merged

EVM: respect prank address in callChecks #379

merged 5 commits into from
Nov 22, 2023

Conversation

d-xo
Copy link
Collaborator

@d-xo d-xo commented Sep 18, 2023

Description

Previously we would incorrectly fail here if sending value after calling vm.prank since callChecks did not take the value of overrideCaller into account when checking if the caller had sufficient balance.

There is some duplication of the logic in transfer here, but I'm not sure how to handle it better, since I do not want transfer to be incomplete, and we can't rely on transfer for callcode and delegatecall (which don't actually move any eth).

This fixes #343.

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

Previously we would incorrectly fail here if sending value after calling
`vm.prank` since `callChecks` did not take the value of `overrideCaller`
into account when checking if the caller had sufficient balance.

There is some duplication of the logic in `transfer` here, but I'm not
sure how to handle it better, since I do not want `transfer` to be
incomplete, and we can't rely on `transfer` for callcode and
delegatecall (which don't actually move any eth).

This fixes #343.
@msooseth
Copy link
Collaborator

I'm re-kicking it, seems to have died during fuzzing, either running out of time or out of stack.

src/EVM.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@msooseth msooseth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@d-xo
Copy link
Collaborator Author

d-xo commented Sep 25, 2023

looks like we're hitting some failures in writeWord on this branch, which is very confusing since I didn't touch that code at all in this pr. I noticed them locally, but couldn't reproduce them consistently enough to debug properly and so assumed they weren't related to my changes and decided to push anyway. I'm now suspicious that they are somehow related since they seem to repro pretty consistenly in ci, and we never see them on main....

@msooseth
Copy link
Collaborator

msooseth commented Nov 2, 2023

I'm re-kicking it. That bug you hit is a randomly occurring, known one, I think. I think it'd be worthwhile to get this merged :) Let's see if it works after re-kick. If it does, I'll merge :)

@d-xo d-xo merged commit e434b8e into main Nov 22, 2023
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

Successfully merging this pull request may close these issues.

prank call check wrong sender balance
2 participants