Skip to content
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

forge script call traces should not print private key. #6995

Closed
2 tasks
robinsdan opened this issue Feb 2, 2024 · 4 comments · Fixed by #7049
Closed
2 tasks

forge script call traces should not print private key. #6995

robinsdan opened this issue Feb 2, 2024 · 4 comments · Fixed by #7049
Assignees
Labels
T-bug Type: bug

Comments

@robinsdan
Copy link

robinsdan commented Feb 2, 2024

Component

Forge

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

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (24abca6 2024-01-16T07:35:41.234348345Z)

What command(s) is the bug in?

forge script

Operating System

None

Describe the bug

The call traces will print the private key param of VM::startBroadcast and VM::broadcast. It may lead to users inadvertently leaking sensitive information.

        vm.startBroadcast(uint256(vm.envBytes32("MAINNET_KEY")));
        // ... 
        vm.stopBroadcast();

@robinsdan robinsdan added the T-bug Type: bug label Feb 2, 2024
@gakonst gakonst added this to Foundry Feb 2, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Feb 2, 2024
@mattsse
Copy link
Member

mattsse commented Feb 2, 2024

good point, @DaniPopes can we redact the envBytes32 call as well?
I assume this is mostly used for reading pk

@zerosnacks
Copy link
Member

Please correct me if I'm wrong but I suspect there is a logical error in:

if !func.inputs.is_empty() && func.inputs[0].ty != "uint256" {

This should be func.inputs[0].ty == "uint256"

Currently both envUint (shown in https://book.getfoundry.sh/cheatcodes/start-broadcast#examples) and envBytes32 print the private key.

Redacting the envUint and envBytes32 directly (as was done for addr: #6698) doesn't seem necessary because it is handled here (<env var value>):

https://github.com/foundry-rs/foundry/blob/master/crates/evm/traces/src/decoder/mod.rs#L462C20-L462C27

Note that there is possibly the same logical error here:

if !decoded.is_empty() && func.inputs[0].ty != "uint256" {

@mattsse
Copy link
Member

mattsse commented Feb 7, 2024

nice find! this makes sense

do you want to submit a fix + test?

@zerosnacks
Copy link
Member

nice find! this makes sense

do you want to submit a fix + test?

Sure! Feel free to assign this to me

mattsse pushed a commit that referenced this issue Feb 8, 2024
* handle edge cases and lay out structure for test

* add various testcases, complete but quite verbose

* change to parameterized test

* remove debug lines

* clean up

* fix linting issue

* fix rustfmt

* address feedback
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Feb 8, 2024
@jenpaff jenpaff moved this from Done to Completed in Foundry Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

3 participants