block.number means different things on different L2s

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/README.md?plain=1#L123

Vulnerability details

Impact

First, would be key to note that protcol has been clearly stated to be deployed on Arbitrum type L2s, source.

Now On Optimism, block.number is the L2 block number, but on Arbitrum, it's the L1 block number, and ArbSys(address(100)).arbBlockNumber() must be used. Furthermore, L2 block numbers often occur much more frequently than L1 block numbers (any may even occur on a per-transaction basis), so using block numbers for timing results in inconsistencies, especially when voting is involved across multiple chains. As of version 4.9, OpenZeppelin has modified their governor code to use a clock rather than block numbers, to avoid these sorts of issues.

Proof of Concept

There are multiple instances of this issue as can be seen using this search command, i.e "implementations of block.number.

Recommended Mitigation Steps

Implement different Clocks, and use the right one on each EVM

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #816

[c4-judge]

Trumpero marked the issue as not a duplicate

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by Trumpero

[c4-judge]

Trumpero marked the issue as duplicate of #816

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[Bauchibred]

HI @Trumpero, this seems to have been wrongly duplicated to 816 and graded as QA, this issue states a different thing and is not about hardcoded block minting time, however stands on it's own and is an issue about the nuances of block.number on arbitrum like L2s.

As hinted in report, this can be seen from the version 4.9 of Openzeppelin, where they modified the governor code to now use clocks in order to curb the inconsistencies that would otherwise be present.

[Tri-pathi]

Agree with @Bauchibred
Hardcoded block minting time and block.number in arbitrum aren't same issue
we have tried to explain in #826

[Trumpero]

The decision to use block number instead of timestamp is intended to standardize the votes by blocks, as vote tokens have checkpoints of balances per block number, not timestamp. The number of blocks for a duration is estimated, and only the voting duration uses block numbers, which is long enough to be safe (7 days). Therefore, the only valuable information in this report is the different block times of different chains, which suggests that the protocol shouldn't hardcode the number of blocks for voting duration. Thus, this should be considered a duplicate of #816 and should be considered as low severity at most.