bug: --gas-report groups all methods of all contracts that use the same type of proxy contract

[sakulstra]

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

macOS (Apple Silicon)

Describe the bug

After trying to generate --json output where i faced #9111 I played a bit around and faced another "bug"? in regards to the output in general.

If you have multiple contracts in a repo using the same type of proxy (e.g. oz TransparentUpgradeableProxy), foundy will group all methods of all contracts using this proxy together. What is even worse, i think when multiple implementations implement the same signature (e.g. transfer - it will show as one and consider all implementations for things like average etc).

---

Not 100% sure what the solution is, perhaps it could be possible to differentiate multiple Proxies dependent on the impl being delegate-called?

[grandizzy]

@sakulstra could you please provide a minimal repro for this case? thank you!

[sakulstra]

@grandizzy not quite minimal, but e.g. here on this repo: https://github.com/aave-dao/aave-v3-origin

The issue is quite easy to reproduce just from pseudo-code:

contract CheapDeposit is Initializable {
  function deposit(uint256 amount) external {
     // doing sth
  }
}

contract ExpensiveDeposit is Initializable {
  function deposit(uint256 amount) external {
     // doing sth
  }
}

contract MyTest {
  function test() {
    CheapDeposit a = CheapDeposit(new Proxy(new CheapDeposit()));
    ExpensiveDeposit b = ExpensiveDeposit(new Proxy(new ExpensiveDeposit()));
    a.deposit(4);
    b.deposit(4);
  }
}

In the --gas-report foundry would now record Proxy deposit(uint256 amount) with mixed gas usage from theCheapDeposit and ExpensiveDeposit implementations.

The solution i think would be "detect calls trough proxies" and separate proxies in the report per underlying implementation.

[grandizzy]

@sakulstra thank you! could you please validate that's the issue you see, with following test grandizzy/invariant-uups@9d75fc2 (forge test --mt test_gas_report --gas-report)

the gas report shows

| lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol:ERC1967Proxy contract |                 |       |        |       |         |
|----------------------------------------------------------------------------------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                                                                                                  | Deployment Size |       |        |       |         |
| 178197                                                                                                                           | 1200            |       |        |       |         |
| Function Name                                                                                                                    | min             | avg   | median | max   | # calls |
| deposit                                                                                                                          | 26476           | 26476 | 26476  | 26476 | 2       |


| test/GasReport.t.sol:CheapDeposit contract |                 |       |        |       |         |
|--------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                            | Deployment Size |       |        |       |         |
| 499201                                     | 2129            |       |        |       |         |
| Function Name                              | min             | avg   | median | max   | # calls |
| deposit                                    | 382             | 382   | 382    | 382   | 1       |
| initialize                                 | 46368           | 46368 | 46368  | 46368 | 1       |


| test/GasReport.t.sol:ExpensiveDeposit contract |                 |       |        |       |         |
|------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                | Deployment Size |       |        |       |         |
| 499201                                         | 2129            |       |        |       |         |
| Function Name                                  | min             | avg   | median | max   | # calls |
| deposit                                        | 382             | 382   | 382    | 382   | 1       |
| initialize                                     | 46368           | 46368 | 46368  | 46368 | 1       |

[sakulstra]

@grandizzy yes that is a reproduction of the issue i'd say.

I should have done the minimal repro myself though as i didn't realize i'm restricting the --gas-report to some specific contracts, so for me CheapDeposit and ExpensiveDeposit did not appear 😅 , sorry for the wasted time with this - this part of the issue is definitely invalid :/

---

Anyhow, in the report you shared you can see the second part of the issue manifest.
The deposit on ERC1967Proxy has recorded 2 calls, and if gas consumption on the two contracts would not be the same, the min/avg/max values would be a mixture of all of them. With the implementations having independent report though i'm not so sure how this "should be handled". From a developer perspective, i guess the thing most interesting here is the cost of fallback.

[grandizzy]

awesome. would it work if we add a prop to gas report to show per called address so then you can see there are 2 proxies / 2 calls, smth like below (we could additionally resolve label per address)? (thinking this should be an opt in to preserve current behavior, @zerosnacks wdyt?)

| lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol:ERC1967Proxy contract |                 |       |        |       |         |
|----------------------------------------------------------------------------------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                                                                                                  | Deployment Size |       |        |       |         |
| 178197                                                                                                                           | 1200            |       |        |       |         |
| Function Name                                                                                                                    | min             | avg   | median | max   | # calls |
| deposit(0x2e234DAe75C793f67A35089C9d99245E1C58470b)                                                                              | 26476           | 26476 | 26476  | 26476 | 1       |
| deposit(0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9)                                                                              | 26476           | 26476 | 26476  | 26476 | 2       |


| test/GasReport.t.sol:CheapDeposit contract             |                 |       |        |       |         |
|--------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                        | Deployment Size |       |        |       |         |
| 499201                                                 | 2129            |       |        |       |         |
| Function Name                                          | min             | avg   | median | max   | # calls |
| deposit(0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f)    | 382             | 382   | 382    | 382   | 1       |
| initialize(0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f) | 46368           | 46368 | 46368  | 46368 | 1       |


| test/GasReport.t.sol:ExpensiveDeposit contract         |                 |       |        |       |         |
|--------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                        | Deployment Size |       |        |       |         |
| 499189                                                 | 2129            |       |        |       |         |
| Function Name                                          | min             | avg   | median | max   | # calls |
| deposit(0xF62849F9A0B5Bf2913b396098F7c7019b51A820a)    | 382             | 382   | 382    | 382   | 2       |
| initialize(0xF62849F9A0B5Bf2913b396098F7c7019b51A820a) | 46368           | 46368 | 46368  | 46368 | 1       |

I think this would be a nice overall addition too, as if you for example have a test like

        CheapDeposit deposit1 = new CheapDeposit();
        deposit1.deposit(1);
        deposit1.deposit(2);
        CheapDeposit deposit2 = new CheapDeposit();
        deposit2.deposit(10);

you could also see them depicted as

| test/GasReport.t.sol:CheapDeposit contract             |                 |       |        |       |         |
|--------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                        | Deployment Size |       |        |       |         |
| 499201                                                 | 2129            |       |        |       |         |
| Function Name                                          | min             | avg   | median | max   | # calls |
| deposit(0xA4AD4f68d0b91CFD19687c881e50f3A00242828c)    | 21586           | 21586 | 21586  | 21586 | 1       |
| deposit(0xc7183455a4C133Ae270771860664b6B7ec320bB1)    | 21586           | 21586 | 21586  | 21586 | 2       |
| initialize(0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f) | 46368           | 46368 | 46368  | 46368 | 1       |

instead

[sakulstra]

I don't really like that.

For non proxy contracts nothing has to change really and adding an address is just confusing.
For proxy contracts the in-test address is not really helpful either.

Imo the two reasonable options are (no idea how possible they are to achieve though)

1. show the fallback cost - as this is actually the interesting part here.

| lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol:ERC1967Proxy contract |                 |       |        |       |         |
|----------------------------------------------------------------------------------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                                                                                                  | Deployment Size |       |        |       |         |
| 178197                                                                                                                           | 1200            |       |        |       |         |
| Function Name                                                                                                                    | min             | avg   | median | max   | # calls |
| fallback                                                                            | 26148           | 26148 | 26148  | 26148 | 2       |

2. indicate the implementation on the method

| lib/openzeppelin-contracts-upgradeable/lib/openzeppelin-contracts/contracts/proxy/ERC1967/ERC1967Proxy.sol:ERC1967Proxy contract |                 |       |        |       |         |
|----------------------------------------------------------------------------------------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                                                                                                  | Deployment Size |       |        |       |         |
| 178197                                                                                                                           | 1200            |       |        |       |         |
| Function Name                                                                                                                    | min             | avg   | median | max   | # calls |
| test/GasReport.t.sol:CheapDeposit.deposit                                                                            | 26476           | 26476 | 26476  | 26476 | 1       |
| test/GasReport.t.sol:ExpensiveDeposit.deposit                                                                            | 26476           | 26476 | 26476  | 26476 | 1       |

[grandizzy]

makes sense. for future reference the issue is we're matching selector against all functions from all contract abis within test here

foundry/crates/evm/traces/src/decoder/mod.rs

Line 361 in f3376a6

let functions = match self.functions.get(selector) {

and should check only for trace.address functions and use fallback as DecodedCallData sig if no match