feat: revert on msg.sender usage inside broadcast in script contracts#13349
Open
feat: revert on msg.sender usage inside broadcast in script contracts#13349
msg.sender usage inside broadcast in script contracts#13349Conversation
When a script uses msg.sender inside a vm.startBroadcast(pk) block where the broadcast signer differs from the default script sender, the script now reverts with a clear error message. This prevents a class of bugs where tokens or values are accidentally sent to the wrong address (Foundry's default sender 0x1804... instead of the intended signer). The check is gated behind script_execution_protection (same as the existing address(this) check) and only triggers when: - CALLER opcode executes in the script contract - A broadcast is active - The broadcast signer differs from tx.origin (default sender) When --sender or --private-key is passed to forge script, msg.sender already matches the broadcast signer, so no revert occurs. Ref: #13346 Amp-Thread-ID: https://ampcode.com/threads/T-019c320e-8bbc-730f-a652-2a2c2fba1714 Co-authored-by: Amp <amp@ampcode.com>
gakonst
force-pushed
the
fix/revert-msg-sender-in-broadcast
branch
from
448eba2 to
bc54e26
Compare
Previously, maybe_load_private_key only checked raw private keys and Turnkey to infer the --sender. This meant hardware wallets (Ledger, Trezor) and cloud KMS signers (AWS, GCP) were not considered, causing msg.sender in scripts to remain the default sender even when exactly one signer was provided. Renamed to maybe_load_sender and now uses Wallets::signers() which includes all wallet types (private keys, keystores, ledger, trezor, AWS KMS, GCP KMS, Turnkey, browser). If exactly one signer exists across all backends, it is used as the script sender. Amp-Thread-ID: https://ampcode.com/threads/T-019c320e-8bbc-730f-a652-2a2c2fba1714 Co-authored-by: Amp <amp@ampcode.com>
- Fix formatting in stack.rs (nightly rustfmt) - Update CheckOverrides test to not use msg.sender inside a startBroadcast(0x1337) block, since the new protection correctly reverts on this pattern Amp-Thread-ID: https://ampcode.com/threads/T-019c320e-8bbc-730f-a652-2a2c2fba1714 Co-authored-by: Amp <amp@ampcode.com>
- vm.broadcast(pk) single-call variant - startBroadcast(address) variant - msg.sender before/after broadcast (allowed) - msg.sender inside deployed contracts called from broadcast (allowed) - tx.origin inside broadcast (allowed, only CALLER is checked) - --sender flag matching broadcast signer (allowed) - startBroadcast() no args, no --sender (allowed, signer == default) Amp-Thread-ID: https://ampcode.com/threads/T-019c320e-8bbc-730f-a652-2a2c2fba1714 Co-authored-by: Amp <amp@ampcode.com>
Replace 'Use vm.addr(<pk>) instead' with 'Use the --sender flag or store the deployer address in a variable instead' to avoid pushing users toward private key injection in scripts. Amp-Thread-ID: https://ampcode.com/threads/T-019c320e-8bbc-730f-a652-2a2c2fba1714 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c320e-8bbc-730f-a652-2a2c2fba1714 Co-authored-by: Amp <amp@ampcode.com>
zerosnacks
requested review from
0xrusowsky,
DaniPopes,
grandizzy,
mattsse,
onbjerg and
zerosnacks
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prevents the class of bugs where
msg.senderin forge scripts resolves to the wrong address inside broadcast blocks.Ref: #13346
Changes
crates/cheatcodes/src/inspector.rsAdded
script_address: Option<Address>field toCheatcodes. Instep(), when theCALLERopcode executes inside the script contract while a broadcast is active andbroadcast.original_origin != broadcast.new_origin, the script reverts with:Same pattern as the existing
address(this)/ADDRESSopcode protection. Gated behindscript_execution_protection.crates/evm/evm/src/inspectors/stack.rsMoved
InspectorStack::script()fromInspectorStackInnertoInspectorStackso it can propagate the script address to bothScriptExecutionInspectorandCheatcodes.crates/script/src/lib.rsReplaced
maybe_load_private_key()withmaybe_load_sender(). The old method only checked raw private keys and Turnkey to infer--sender, so Ledger, Trezor, AWS KMS, GCP KMS, keystore, and browser wallet users would always get the default sender even with a single signer. Now usesWallets::signers()which includes all wallet backends.Removes the
alloy-signerdependency fromforge-script.crates/forge/tests/cli/script.rsTwo new tests:
should_revert_on_msg_sender_in_broadcast—msg.senderinsidestartBroadcast(pk)reverts; disabled withscript_execution_protection = falseshould_allow_msg_sender_in_broadcast_when_matching—msg.senderinsidestartBroadcast()with--private-keypasses (sender matches)testdata/default/cheats/Broadcast.t.solUpdated
CheckOverrides: removedmsg.sender/tx.originassertions from inside thestartBroadcast(0x1337)block since these now correctly revert.