crytic / medusa

Parallelized, coverage-guided, mutational Solidity smart contract fuzzing, powered by go-ethereum
https://www.trailofbits.com/
GNU Affero General Public License v3.0
287 stars 34 forks source link

`hevm.prank` does not reduce balance of the actor #398

Open boo-0x opened 2 months ago

boo-0x commented 2 months ago

Hi. I am using version 0.1.3 and seems that there is a bug when hevm.prank is used, as after transferring ETH from the account pranked, its balance does not change. The following test succeeds with Echidna and Foundry, but fails with Medusa.

event LogBalanceChange(uint256 balanceChange);

contract Tester {
    function sendETH() public payable {
        uint256 balanceBefore = msg.sender.balance;
        assert(balanceBefore > 10000);

        address payable recipient = msg.sender == address(0x30000) 
            ? payable(address(0x10000)) : payable(address(0x30000));
        hevm.prank(msg.sender);
        recipient.transfer(1000);

        uint256 balanceChange = balanceBefore - msg.sender.balance;
        emit LogBalanceChange(balanceChange);

        assert(balanceChange > 0);
    }
}

Logs:

     => [event] LogBalanceChange(0)
     => [panic: assertion failed]
anishnaik commented 1 month ago

Thanks for reporting @boo-0x - will take a look at this

anishnaik commented 3 weeks ago

Hey @boo-0x just did some investigating. Tbh I am not sure if we can fix this in the short-term. This will probably just be a limitation of using prank in medusa. Here is the context:

Basically, prank works by changing the msg.sender for the next contract call. However, it only works if the call is made to a contract, not an EOA. Since there is no code to execute, we cannot trigger the spoofing of the msg.sender in the next call scope. I tried some hacks to make this work for simple value transfers and none of them seem to be ideal.

TLDR; prank will not work for ether transfers to an EOA.

I will keep investigating but in the short-term we will document this limitation and i'll keep thinking of workarounds.

boo-0x commented 3 weeks ago

Hey @anishnaik thank you for taking the time to look into it.

In fact I first detected the issue interacting with a contract. I have tried now interacting with a contract in different ways and none of them changes the balance either. I am using v0.1.6.

event LogBalanceChange(uint256 balanceChange);

contract Recipient {
    function receiveInFunc() public payable {}
    receive() external payable {}
}

contract Tester {
    function sendETH() public payable {
        Recipient recipient = new Recipient();
        uint256 balanceBefore = msg.sender.balance;

        vm.prank(msg.sender);
        payable(address(recipient)).transfer(1000);
        vm.prank(msg.sender);
        payable(address(recipient)).send(1000);
        vm.prank(msg.sender);
        recipient.receiveInFunc{value: 1000}();
        vm.prank(msg.sender);
        address(recipient).call{value: 1000}("");

        uint256 balanceChange = balanceBefore - msg.sender.balance;
        emit LogBalanceChange(balanceChange);

        assert(balanceChange > 0);
    }
}

Console output:

         => [event] LogBalanceChange(0)
         => [panic: assertion failed]
0xalpharush commented 1 week ago

Maybe we should do something like this https://github.com/ethereum-optimism/op-geth/commit/7a8962c1f1df397835ac6d16abb3877b0ac6df96