crytic / medusa

Parallelized, coverage-guided, mutational Solidity smart contract fuzzing, powered by go-ethereum
https://www.trailofbits.com/
GNU Affero General Public License v3.0
273 stars 33 forks source link

Improve logging API and usage in the cmd package #300

Open anishnaik opened 4 months ago

anishnaik commented 4 months ago
          Yeah probably. I've had a thought while reviewing some logging code that some of the CLI entry point logging could use a bit of cleanup and maybe our logging API should be reworked so the Info/Error/other log functions maybe are a bit more intuitive. I have some ideas we can follow up on in another PR anyways, so I'm probably less concerned about this.

But yeah idk, my thoughts are here, maybe it makes sense to log to it in case we access the logger elsewhere or for some other purpose (would this even be useful to us if we did?). :shrug:

_Originally posted by @Xenomega in https://github.com/crytic/medusa/pull/216#discussion_r1494779445_

Xenomega commented 4 months ago

More loose thoughts:

anishnaik commented 4 months ago

In the event of errors, a stack trace during panic is not always included, but it probably makes sense that kind of info to be logged in a panic, it's more interesting to have in logs than other data.

For error stack traces, we can use this PR and refactor it: https://github.com/crytic/medusa/pull/108

We cannot use the standard library's errors and instead have to import pkg/errors.

Some of the verbose output including module maybe isn't worth keeping if we don't expand this, especially if we have stack traces for errors.

Agreed

The main entry points (CLI/cmd) duplicates some error messages we could probably wrap. e.g. We log an error, then return a different one that looks similar.

This has also introduced a bug because if you set NoColor to true, the CLI output from the cmd package will still be colorized bc we haven't processed the config values yet.

aviggiano commented 2 months ago

Hey

Another request would be to format addresses with Mixed-case checksum encoding (ERC-55)

Right now, copy/pasting Medusa's logs to Solidity does not compile. Example:

1) CryticTester.deposit(address,uint256)(0x470c8b323b89c05bbb0f2a85d733cc7004883d14, 45) (block=2, time=159480, gas=12500000, gasprice=1, value=0, sender=0x0000000000000000000000000000000000010000)
2) CryticTester.withdraw(address,uint256)(0x00000000000000000000000000000000002fef7c, 70880928519027259232681487313122362188527844215780522898084047764600190669520) (block=12543, time=672109, gas=12500000, gasprice=1, value=0, sender=0x0000000000000000000000000000000000010000)
    function test_CryticToFoundry_12() public {
        deposit(0x470c8b323b89c05bbb0f2a85d733cc7004883d14, 45);
        withdraw(0x00000000000000000000000000000000002fef7c, 70880928519027259232681487313122362188527844215780522898084047764600190669520);
    }
This looks like an address but has an invalid checksum. Correct checksummed address: "0x470c8B323b89c05bbB0F2a85d733Cc7004883d14". If this is not used as an address, please prepend '00'. For more information please see https://docs.soliditylang.org/en/develop/types.html#address-literalssolidity(9429)