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

fix: cheatcode depth fixes #1215

Merged
merged 2 commits into from
Apr 7, 2022
Merged

fix: cheatcode depth fixes #1215

merged 2 commits into from
Apr 7, 2022

Conversation

onbjerg
Copy link
Collaborator

@onbjerg onbjerg commented Apr 6, 2022

Reasoning about depth

  • call is called before the depth is increased. This effectively means that depth in call is the depth of the current call, and depth + 1 is the depth of the call we are about to execute.
  • call_end is called after the depth is decreased. This effectively means that depth + 1 is the depth of the call that just finished executing

Implications:

  • If depth == 0, then the root call (depth 1 during execution, but depth 0 in call/call_end) is terminating
  • depth + 1 represents the depth of the call that is about to execute in call, and the depth of the call that just finished executing in call_end
  • depth - 1 represents the depth at which the current call will terminate in call and call_end
  • The call that is about to execute will terminate at depth
  • depth in calls to cheatcodes represent the depth of the current call, not the depth of the call to the cheatcode.

Illustrated, if you have:

function testSomething() {
  vm.expectRevert();
  somewhere.something();
}

Then:

  • Inspectors' call methods are invoked with depth == 0 when we first call testSomething
  • Inspectors' call methods are invoked with depth == 1 when we call the cheatcode
  • Inspectors' call_end methods are invoked with depth == 1 when the call to the cheatcode has finished
  • Inspectors' call methods are invoked with depth == 1 when we call somewhere.something()
  • Inspectors' call_end methods are invoked with depth == 1 when the somewhere.something() call has finished
  • Inspectors' call_end methods are invoked with depth == 0 when the testSomething call has finished

expectEmit bug (#1214)

This means that if we do:

function testSomeEmit() {
  vm.expectEmit(..);
  emit Something(somewhere.someValue());
  emitter.emit();
}

Then:

  • Inspectors' call methods are invoked with depth == 0 when we first call testSomeEmit
  • Inspectors' call methods are invoked with depth == 1 when we call the cheatcode
    • We then push an expected emit at depth == 1
  • Inspectors' call_end methods are invoked with depth == 1 when the call to the cheatcode has finished
    • The expected emit is ignored by the call_end handler since we just exited a cheatcode call
  • Inspectors' call methods are invoked with depth == 1 when we call somewhere.someValue()
  • Inspectors' call_end methods are invoked with depth == 1 when the somewhere.someValue() call has finished
    • The expected emit is checked, and we revert, even when we should not.

In fact we should test for the expected emit at depth - 1. In other words, when the call from which the cheatcode was invoked ends.

tx.origin bug (#1210)

Whenever call is invoked, we check if the depth is greater than or equal to the depth at which the cheatcode was invoked. If it is, then we check:

  • If depth is exactly the depth at which the cheatcode was invoked, then we set msg.sender. This is sound because we only want to prank the first msg.sender, not the nested ones in case of nesting. If the depth is greater than or equal to the depth at which the cheatcode was invoked, we set tx.origin, which is sound, because we want this to persist even in the case of nesting.

Whenever call_end is invoked, we check if we have an ongoing prank. If we do, then we check if the prank is for one call only. If it is, then we erase the prank from memory, so we don't set msg.sender in subsequent calls. This is sound. However, we always reset tx.origin to the original value, regardless of the depth of the call that is terminating. This is sound when we use startPrank, since it will just continually reapply tx.origin, but for prank this is unsound. We should only do that when we are back at the depth at which the prank was started.

Closes #1214
Closes #1210

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Clean - also great example of how a proper bugfix PR should be (clear description of the issue and regression test)

@gakonst gakonst merged commit 588b05b into master Apr 7, 2022
@gakonst gakonst deleted the onbjerg/depth-fixes-the-sequel branch April 7, 2022 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
2 participants