Skip to content

feat(script): revert if address(this) used #10295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Apr 23, 2025
Merged

Conversation

gap-editor
Copy link
Contributor

Motivation

Closes #10289

Users can accidentally use address(this) within forge script contracts, often intending to refer to the script contract itself for operations like transferring ownership or sending tokens during deployment. However, the address(this) in a script context refers to a temporary, locally deployed contract instance used for the simulation. This address does not exist on the target network and relying on it leads to deployment errors or incorrect state on-chain.

This PR introduces a warning mechanism to alert users when address(this) is used during a forge script execution, helping prevent these common mistakes.

Solution

  1. Introduced ScriptAddressWarnInspector:

    • Defined a new revm::Inspector implementation named ScriptAddressWarnInspector within crates/evm/evm/src/inspectors/stack.rs.
    • This inspector implements the step method, which checks if the currently executing EVM opcode is ADDRESS (0x30).
    • If the ADDRESS opcode is detected, it prints a warning message to stderr: "forge script warning: Usage of \address(this)` detected. Script contracts are ephemeral and their addresses should not be relied upon."`
  2. Integrated into InspectorStack:

    • Added an optional address_warn_inspector field to the InspectorStackInner struct.
    • Added a corresponding address_warn boolean flag and a builder method .address_warn_inspector(bool) to InspectorStackBuilder.
    • Updated the InspectorStackBuilder::build method to instantiate ScriptAddressWarnInspector if the address_warn flag is true.
    • Modified the Inspector implementation for InspectorStackRefMut to invoke the step method of the address_warn_inspector if it's present.
  3. Enabled for forge script:

    • In crates/script/src/lib.rs, within the _get_runner function (which configures the Executor for scripts), modified the ExecutorBuilder chain to call .address_warn_inspector(true). This ensures the warning inspector is always active during forge script runs.

This approach leverages the existing revm inspector system to passively detect the opcode usage without significantly impacting performance and provides a clear warning directly to the user during script execution.

PR Checklist

  • Added Tests
  • Added Documentation
  • No Breaking changes

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very supportive, some questions.

wdyt @klkvr @grandizzy

klkvr
klkvr previously requested changes Apr 12, 2025
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you pls add a test too?

@gap-editor gap-editor marked this pull request as draft April 12, 2025 11:13
@gap-editor gap-editor requested review from klkvr and mattsse April 12, 2025 11:42
@gap-editor gap-editor marked this pull request as ready for review April 13, 2025 11:29
@jenpaff jenpaff moved this to Ready For Review in Foundry Apr 15, 2025
@jenpaff jenpaff added this to the v1.2.0 milestone Apr 15, 2025
@grandizzy
Copy link
Collaborator

grandizzy commented Apr 16, 2025

hey @gap-editor I pushed some cleanups in https://github.com/gap-editor/foundry/commit/8fd0fbf5d06a4e548dcefaa6d480ca087af6e752 pls check. thank you!

@gap-editor
Copy link
Contributor Author

@klkvr did i do good?

@grandizzy
Copy link
Collaborator

@klkvr did i do good?

@gap-editor we should keep the changes in script inspector only and not add custom logic in stack. For this case, since when calling libraries the bytecode address is the library address and not script's contract, we could add one more check to verify that interpreter.contract.bytecode_address is the same as script address, pushed a commit to ad such in 153f984

@grandizzy grandizzy dismissed stale reviews from klkvr and mattsse April 17, 2025 07:54

stale

@grandizzy
Copy link
Collaborator

grandizzy commented Apr 17, 2025

should be rfr, @mattsse @klkvr @DaniPopes pls check (any perf improvement that could be done?)

@grandizzy grandizzy changed the title feat(script): add warning for address(this) usage feat(script): revert if address(this) used Apr 17, 2025
@zerosnacks
Copy link
Member

lgtm! Small notes in regards to output mode for the error

@grandizzy grandizzy requested a review from zerosnacks April 22, 2025 14:21
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

ci error is unrelated

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@grandizzy grandizzy self-assigned this Apr 22, 2025
@grandizzy grandizzy merged commit 055b0ea into foundry-rs:master Apr 23, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from Ready For Review to Done in Foundry Apr 23, 2025
@grandizzy grandizzy added T-likely-breaking Type: requires changes that can be breaking T-feature Type: feature C-forge Command: forge labels Apr 23, 2025
@jenpaff jenpaff moved this from Done to Completed in Foundry Apr 23, 2025
@frolic
Copy link

frolic commented Apr 30, 2025

Any chance this behavior can be put behind a flag? We use address(this) a lot throughout MUD to determine the calling context and this error now keeps us from using ~any scripts.

@grandizzy
Copy link
Collaborator

grandizzy commented Apr 30, 2025

Any chance this behavior can be put behind a flag? We use address(this) a lot throughout MUD to determine the calling context and this error now keeps us from using ~any scripts.

yes, that will be available soon with #10408

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature T-likely-breaking Type: requires changes that can be breaking
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

feat(forge script): add warning if address(this) is used
8 participants