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

bug(forge): msg.sender set using startPrank in another contract is wrong #5521

Closed
1 of 2 tasks
minaminao opened this issue Aug 1, 2023 · 4 comments · Fixed by #10018
Closed
1 of 2 tasks

bug(forge): msg.sender set using startPrank in another contract is wrong #5521

minaminao opened this issue Aug 1, 2023 · 4 comments · Fixed by #10018
Assignees
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge P-normal Priority: normal T-bug Type: bug
Milestone

Comments

@minaminao
Copy link
Contributor

minaminao commented Aug 1, 2023

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (ca67d15 2023-08-01T22:03:12.511500000Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Intel)

Describe the bug

msg.sender set using startPrank in another contract is wrong

The bug reported in #5515 was fixed in #5520 (thanks for the quick fix), but this is not a complete fix.

Using startPrank instead of the banned prank still has the bug.

The following test

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, Vm} from "forge-std/Test.sol";

contract ContractTest is Test {
    function test() public {
        address player = makeAddr("player");
        SenderLogger senderLogger = new SenderLogger();
        Contract c = new Contract();

        senderLogger.log(); // Log(ContractTest, DefaultSender)
        vm.startPrank(player, player);
        senderLogger.log(); // Log(player, player)
        c.f(); // vm.startPrank(player)
        senderLogger.log(); // Log(ContractTest, player) <- ContractTest should be player
        vm.stopPrank();
    }
}

contract Contract {
    Vm public constant vm = Vm(address(bytes20(uint160(uint256(keccak256("hevm cheat code"))))));

    function f() public {
        vm.startPrank(msg.sender);
    }
}

contract SenderLogger {
    event Log(address, address);

    function log() public {
        emit Log(msg.sender, tx.origin);
    }
}

outputs

$ forge test -vvvvv
(snip)
Running 1 test for test/Counter.t.sol:ContractTest
[PASS] test() (gas: 162948)
Traces:
  [162948] ContractTest::test() 
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C]
    ├─ [0] VM::label(player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C], player) 
    │   └─  ()
    ├─ [33087] → new SenderLogger@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   └─ ← 165 bytes of code
    ├─ [54305] → new Contract@0x2e234DAe75C793f67A35089C9d99245E1C58470b
    │   └─ ← 271 bytes of code
    ├─ [1451] SenderLogger::log() 
    │   ├─ emit Log(: ContractTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], : DefaultSender: [0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38])
    │   └─  ()
    ├─ [0] VM::startPrank(player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C], player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C]) 
    │   └─  ()
    ├─ [1451] SenderLogger::log() 
    │   ├─ emit Log(: player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C], : player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C])
    │   └─  ()
    ├─ [472] Contract::f() 
    │   ├─ [0] VM::startPrank(player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C]) 
    │   │   └─  ()
    │   └─  ()
    ├─ [1451] SenderLogger::log() 
    │   ├─ emit Log(: ContractTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], : player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C])
    │   └─  ()
    ├─ [0] VM::stopPrank() 
    │   └─  ()
    └─  ()

In Contract::f(), VM::startPrank(player: [0x44E97aF4418b7a17AABD8090bEA0A471a366305C]) is execute, but the next msg.sender is ContractTest, not player.

Full source: https://github.com/minaminao/foundry-bug-reports/tree/main/2023-08-broken-prank-2

@minaminao minaminao added the T-bug Type: bug label Aug 1, 2023
@gakonst gakonst added this to Foundry Aug 1, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Aug 1, 2023
@Evalir
Copy link
Member

Evalir commented Aug 1, 2023

Right, this is actually not necessarily a bug, but rather a footgun—the msg.sender is being reset to ContractTest because the call depth where it was set has exited, but technically there is a running prank at the test level. How should we handle this @mds1 ? We have two options here i believe:

  • have a "stack" system so we can restore pranks at earlier call depths (this is what i'd prefer)
  • disallow setting vm.startPrank at a different depth from where it was set (i can see use cases for this though)

@minaminao
Copy link
Contributor Author

Fwiw, I found this when I used prank in a Huff/Vyper deployer contract (at a different depth). foundry-huff uses the same method: https://github.com/huff-language/foundry-huff/blob/bcac40d5b0950588b30cdf24dff25df080007295/src/HuffConfig.sol#L217 (this uses prank, not startPrank though)

@zerosnacks zerosnacks added T-feature Type: feature and removed T-bug Type: bug labels Jul 3, 2024
@zerosnacks zerosnacks changed the title bug(forge): msg.sender set using startPrank in another contract is wrong feat(forge): add a "stack" system so pranks can be restored at earlier call depths Jul 3, 2024
@zerosnacks zerosnacks changed the title feat(forge): add a "stack" system so pranks can be restored at earlier call depths feat(forge): add a stack system so pranks can be restored at earlier call depths Jul 3, 2024
@zerosnacks zerosnacks added C-forge Command: forge A-cheatcodes Area: cheatcodes labels Jul 3, 2024
@zerosnacks
Copy link
Member

Turning this into a feature request of adding a stack system so pranks can be restored at earlier call depths as suggested by @Evalir

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@grandizzy grandizzy changed the title feat(forge): add a stack system so pranks can be restored at earlier call depths bug(forge): msg.sender set using startPrank in another contract is wrong Oct 10, 2024
@grandizzy grandizzy added T-bug Type: bug and removed T-feature Type: feature labels Oct 10, 2024
@grandizzy
Copy link
Collaborator

As discussed internally we're looking to make this consistent with starting a new prank at the same depth (that is to error that a prank is already started)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge P-normal Priority: normal T-bug Type: bug
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

4 participants