matter-labs / foundry-zksync

Fork of Foundry tailored for zkSync environment
Apache License 2.0
299 stars 130 forks source link

Reading value that was deleted in a mapping causes panic #478

Closed alexkeating closed 2 months ago

alexkeating commented 4 months ago

Component

Forge

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

What version of Foundry are you on?

No response

What command(s) is the bug in?

No response

Operating System

None

Describe the bug

Description

It looks like when I read a empty value on a mapping my test panics. I am unsure what exactly is going on, but I can reproduce with the steps below.

  1. Checkout this branch locally https://github.com/alexkeating/hats-protocol/tree/experimentation/why-not-working
  2. Run the following test forge test --match-test testAdminCanBurnAndRemintLinkedTopHat --zksync

I added console logs throughout and it looks like the issue is happening in this test in Hats.sol on line 1341.

Karrq commented 3 months ago

Hey thanks for the report! We are looking into this and will let you know when we have a fix ready.

Thanks also for the example, and I'd really appreciate if you could list the logical list of operations that triggers this failure: from what I can tell, it's something like "populate slot" (mint) -> "delete storage slot" (burn) -> "read deleted storage slot" (as part of reminting), correct?

I'd like to further reduce the example to something simpler (if possible) to make debugging more effective, perhaps something we can integrate in our test suite :)

nbaztec commented 3 months ago

@alexkeating we investigated the issue in detail and figured out that this weird behavior was caused by a combination of zkEVM call behavior for missing code addresses (mocked), and using low level calls. Basically the following mocks were missing from your setup:

vm.mockCall(
    _eligibility,
    abi.encodeWithSignature("getWearerStatus(address,uint256)", topHatWearer, topHatId),
    abi.encode()
);
vm.mockCall(
    _eligibility,
    abi.encodeWithSignature("getWearerStatus(address,uint256)", topHatWearer, secondHatId),
    abi.encode()
);
vm.mockCall(
    _eligibility,
    abi.encodeWithSignature("getWearerStatus(address,uint256)", address(99), secondTopHatId),
    abi.encode()
);

In the EVM, calling a non-existent address returns the zero-value but on zkEVM it causes unexpected behavior which manifested itself in this way when using low-level calls.

As a mitigation we will add some error logs when we encounter this behavior, and potentially raise an issue with zkEVM to fail more reliably.

PS: Please note that addresses below 65536 (0 - 65535) are reserved by zksync and may not be used in tests.

Karrq commented 2 months ago

Hey @alexkeating, I'll go ahead and mark this issue as completed - we introduced some heuristic and logging to better detect this case in the future, and it should also give you enough information to troubleshoot our test and add the necessary mockCall invocations.

Feel free to reopen the issue in case there's more to add or if this didn't solve your issue, thanks!

alexkeating commented 2 months ago

Great, thank you guys!