Don't keep CALLER balance as U256::MAX

[mds1]

Component

Forge

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

• Foundry
• Foundryup

What version of Foundry are you on?

No response

What command(s) is the bug in?

No response

Operating System

No response

Describe the bug

Here, we set the balance of CALLER and other addresses to U256::MAX to make sure they can execute deploys.

foundry/forge/src/runner.rs

Lines 69 to 71 in 7e12adb

	// We max out their balance so that they can deploy and make calls.
	self.executor.set_balance(self.sender, U256::MAX);
	self.executor.set_balance(CALLER, U256::MAX);

A few lines below, we set the balances to the user-specified initial balance.

foundry/forge/src/runner.rs

Lines 128 to 131 in 7e12adb

	// Now we set the contracts initial balance, and we also reset `self.sender`s balance to
	// the initial balance we want
	self.executor.set_balance(address, self.initial_balance);
	self.executor.set_balance(self.sender, self.initial_balance);

However, CALLER is left out of there, so if a fuzz test chooses the CALLER address and that test sends ETH to that address, the test reverts with balance overflow.

Proposed fix is to set the CALLER balance after L132 to either 0, or to self.initial_balance. I believe either should be suitable since CALLER isn't used afterwards AFAIK.

This is partially related to #2322 in that I don't think the default balance of an account should ever be kept at U256::MAX, so in that PR I'd suggest using the default self.initial_balance that's typically used instead.

[mattsse]

sounds reasonable,

let's set it to self.initial_balance again, setting it to 0 could have side-effects if sender == CALLER I think

[mds1]

@jferas pointed out to me this happens with another address too:

foundry/evm/src/executor/mod.rs

Lines 108 to 115 in 71541ee

	let creator = "0x3fAB184622Dc19b6109349B94811493BF2a45362".parse().unwrap();
	self.set_balance(creator, U256::MAX);
	self.deploy(
	creator,
	hex::decode("604580600e600039806000f350fe7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe03601600081602082378035828234f58015156039578182fd5b8082525050506014600cf3").expect("Could not decode create2 deployer init_code").into(),
	U256::zero(),
	None
	)?;

I wonder if instead the right solution is to exclude these addresses from the fuzzer dict? 0x3fAB184622Dc19b6109349B94811493BF2a45362 definitely seems safe to exclude because it seems it's only used to deploy the create2 contract, but CALLER maybe is used elsewhere and still should be included?

Either way, 0x3fAB184622Dc19b6109349B94811493BF2a45362 definitely doesn't need a balance of U256::MAX. Let me know what you all think

[onbjerg]

Unsure, I don't know what that address is being used for. I think it's for script?

[mattsse]

that sounds right, wdyt @joshieDo

while we're at it, can we make these constants?

[onbjerg]

Reopened because I don't think it resolved #2390 (comment)

[mds1]

Yep, thanks. Interestingly, @jferas and I both did not see fuzz failures with either of those two addresses until recently, but I don't recall seeing a change that would cause them to start being included in the fuzz dict.

So I think the scope of this issue may be three items:

1. Reduce the initial balance of 0x3fAB184622Dc19b6109349B94811493BF2a45362 to something smaller.
2. Make these addresses constants
3. Decide if these addresses should really be in the fuzz dict or not

Let me know if that sounds right

[onbjerg]

Hmm, I think self.sender and self.test_address could both be valid inputs(?) and should not be excluded. For CALLER I think we should not have any balance on it before it is actually used - as far as I remember it is only used to call failed() on the test contracts? So potentially CALLER could have it's balance increased in Executor::is_success before the call, and reduced to 0 after the call

[mds1]

Hmm, I think self.sender and self.test_address could both be valid inputs(?) and should not be excluded.

Yea I think I lean this way also, just want to make sure they have more realistic default balances in that case

For CALLER I think we should not have any balance on it before it is actually used - as far as I remember it is only used to call failed() on the test contracts?

Ah ok, yea that seems like a reasonable approach to me then. Perhaps we keep the balance zero, set it to nonzero to call failed(), then reset back to zero after the call?

[onbjerg]

Ah ok, yea that seems like a reasonable approach to me then. Perhaps we keep the balance zero, set it to nonzero to call failed(), then reset back to zero after the call?

Yea that's what I meant

Yea I think I lean this way also, just want to make sure they have more realistic default balances in that case

Afaik both balances add up to U256::max so it should be safe unless explicitly increased in foundry.toml beyond that. However I do wonder if DappTools used to set the balance of the test caller itself to something other than 0? It would be nice if we could do that but afaik it's not really possible since REVM always does gas accounting and checks the calling account has a balance - if we do the same trick with setting balance > 0 for self.sender before we call a test function and set it to 0 afterwards it wouldn't mean anything since the balance would still be set for the test lol

[joshieDo]

0x3fAB184622Dc19b6109349B94811493BF2a45362

Does it still have issues with the latest version? Current master branch should handle that

foundry/evm/src/executor/mod.rs

Lines 108 to 120 in f0fa5da

	let creator = "0x3fAB184622Dc19b6109349B94811493BF2a45362".parse().unwrap();
	// Probably 0, but just in case.
	let initial_balance = self.get_balance(creator);
	self.set_balance(creator, U256::MAX);
	self.deploy(
	creator,
	hex::decode("604580600e600039806000f350fe7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe03601600081602082378035828234f58015156039578182fd5b8082525050506014600cf3").expect("Could not decode create2 deployer init_code").into(),
	U256::zero(),
	None
	)?;
	self.set_balance(creator, initial_balance);

[onbjerg]

The ask is to set it to 0 again after deployment 😄

[klkvr]

Closed by #2322 and #8331