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

[minaminao]

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

[Evalir]

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]

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]

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

[grandizzy]

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)