foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.29k stars 1.75k forks source link

Foundry: Fork state used if storage slot is cleared #1894

Closed bernard-wagner closed 2 years ago

bernard-wagner commented 2 years ago

Component

Forge, Cast, Anvil

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

What version of Foundry are you on?

anvil 0.1.0 (23f7db9 2022-06-08T18:32:28.171151Z)

What command(s) is the bug in?

anvil, cast run

Operating System

Linux

Describe the bug

Interesting behaviour between Anvil and cast run when using forked state. If a storage slot gets cleared, the ForkedDB will revert to using the cache and not the reset value. This is because revm removes the entry if the value is set to 0. ForkedDB looks for the existence of the entry to determine whether to use fork cache or in memory db.

A temporary fix to test the theory:

diff --git a/crates/revm/src/db/in_memory_db.rs b/crates/revm/src/db/in_memory_db.rs
index 725473b..f7201ee 100644
--- a/crates/revm/src/db/in_memory_db.rs
+++ b/crates/revm/src/db/in_memory_db.rs
@@ -97,11 +97,7 @@ impl<ExtDB: DatabaseRef> DatabaseCommit for CacheDB<ExtDB> {
                     storage.clear();
                 }
                 for (index, value) in acc.storage {
-                    if value.is_zero() {
-                        storage.remove(&index);
-                    } else {
-                        storage.insert(index, value);
-                    }
+                    storage.insert(index, value);
                 }
                 if storage.is_empty() {
                     self.storage.remove(&add);

Test scenario

cast run  0xeea7f8b87ea2ba65df2f2217aafa6b75de8ac02cad7d27f8c089c24778fed35a
Executing previous transactions from the block.
Traces:
  [11152] 0xc02a…6cc2::2e1a7d4d(00000000000000000000000000000000000000000000000036adb11fff0a0000) 
    ├─ [0] 0x5370…79a1::fallback{value: 3940000000000000000}() 
    │   └─ ← ()
    ├─  emit topic 0: 0x7fcf532c15f0a6db0bd6d0e038bea71d30d808c7d98cb3bf7268a95bf5081b65
    │       topic 1: 0x000000000000000000000000537038d516e7e71bff78a555799ce0daa01e79a1
    │           data: 0x00000000000000000000000000000000000000000000000036adb11fff0a0000
    └─ ← ()

Script ran successfully.
Gas used: 30404

This transaction should fail as there is a previous transaction in the block from the same caller that already withdraws the WETH:

cast receipt 0xeea7f8b87ea2ba65df2f2217aafa6b75de8ac02cad7d27f8c089c24778fed35a
blockHash               0xcfc76595e43d4dd9586c4beedf20fc5bebe6b669166eb41135dc85f70bbf1054
blockNumber             14047503
contractAddress         
cumulativeGasUsed       22811249
effectiveGasPrice       142910302616
gasUsed                 23731
logs                    []
logsBloom               0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
root                    
status                  0
transactionHash         0xeea7f8b87ea2ba65df2f2217aafa6b75de8ac02cad7d27f8c089c24778fed35a
transactionIndex        421
type                    2
mattsse commented 2 years ago

good find!

@rakita wdyt about also keeping 0 values in the account storage, instead of removing them? would this cause any side effects?

rakita commented 2 years ago

@mattsse makes sense to leave the default value. It will not disrupt anything in evm as commit is postprocessing. This is needed for both storage and cache (accounts) @bernard-wagner Thank you, Sir

rakita commented 2 years ago

Here change in revm CacheDB, it is now aware if account was cleared(deleted) or not and storage will be removed if needed, https://github.com/bluealloy/revm/pull/140