Literal block numbers used to represent time will produce different time intervals across different blockchains

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L36
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/GuildVetoGovernor.sol#L230-L232

Vulnerability details

Description

The ECG code is meant to be deployed on Ethereum and across L2s like Arbitrum. It is expected that they should be EVM compliant, with some differences in the way the chains operate. One of such differences is the block time. Arbitrum currently has a block time of ~0.26 seconds while Ethereum currently has a block time of ~13 seconds. This means they'll produce one block every ~0.26 seconds and ~13 seconds respectively. The number of blocks a chain produces can be used to approximate time.

E.g. for a year Arbitrum will produce approximately 121,292,307 blocks (365 * 24 * 60 * 60 / 0.26 ) while Ethereum will produce approximately 2,425,846 blocks (365 * 24 * 60 * 60 / 13).

These no. of blocks can be used to represent time in smart contract. But a problem will arise when a smart contract to be deployed across multiple chains uses the same no. of blocks to represent time on these two chains. This will lead to a shorter time on the chains with faster block time and a longer time on chains with slower block times. This issue arises in two places in the ECG code:

1. The GuildVetoGovernor voting period.

function votingPeriod() public pure override returns (uint256) {
    return 2425847; // ~1 year with 1 block every 13s
}

The no. of blocks here is meant to represent a year on Ethereum mainnet but the code will also be deployed on Arbitrum. This will be 7 days (2425847 * 0.26 / (60 * 60 * 24)) on Arbitrum.

The GuildVetoGovernor is meant to veto proposals currently on delay in the GuildTimelockController contract. The delay shouldn't be up to a year so the voting period of a year on GuildVetoGovernor should be flexible with any delay on GuildTimelockController. But on Arbitrum the block number produces 7 days which may be too low and inflexible with various delay times on GuildTimelockController. But with the implementation of GuildVetoGovernor users can still vote even when the voting period has ended and the proposal can be executed when a quorum is reached, so this does not bring up any significant issue.

2. LendingTermOffboarding POLL_DURATION_BLOCKS state variable.

uint256 public constant POLL_DURATION_BLOCKS = 46523; // ~7 days @ 13s/block

The POLL_DURATION_BLOCKS variable determines how long a poll to offboard a lending term should last. The current value allows it to be 7 days on Ethereum mainnet, but this is 3 hours (46523 * 0.26/ (60 * 60)) on Arbitrum. This time is too short and would mean proposals to remove a lending term can be:

Brought up every 3 hours.

    function proposeOffboard(address term) external whenNotPaused {
        ...
        require(
            block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll active"
        );
        ...

The duration to vote for that proposal would last only 3 hours.

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        require(
            block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll expired"
        );
        ...

Impact

Far less time will be used than anticipated when the same no. of blocks used to represent time on Ethereum mainnet is used on L2s like Arbitrum. Currently this only makes the voting period for offboarding lending terms shorter on chains with faster block times.

Proof of Concept

This shows how the time for voting is far less than expected on LendingTermOffboarding.

1. Users propose to offboard a lending term on Arbitrum
2. Potential supporters expect it to last for 7 days.
3. Anyone that tries to vote after 3 hours will be surprised to find out that the voting period has passed.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Update the block numbers to be set through state variables. This will prompt deployers to always change them when deploying on different chains.

Assessed type

Timing

[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 changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-b

[c4-judge]

Trumpero marked the issue as grade-c

[nonseodion]

Hi @Trumpero, thanks for the judging so far.

I'd like to draw your attention to this finding. It's currently marked as a grade-c. Any reason why it's so would be greatly appreciated.

When compared with the report it duplicates, #816, they both clearly state that the constants, POLL_DURATION_BLOCKS and the return value of votingPeriod will not yield the intended times on blockchains with different block times than Ethereum. But that one is marked with a grade-b and this one a grade-c.

Unlike #1034 which only mentions POLL_DURATION_BLOCKS , this finding mentions POLL_DURATION_BLOCKS and votingPeriod. But this finding gets a grade-c while #1034 gets a grade-a.

I'd appreciate it if you could clear this discrepancy.

[Trumpero]

@nonseodion When an issue is downgraded to QA, it will be counted in the QA report of the warden. After judging QA reports, including all downgraded issues, those downgraded issues of that warden will be given the same grade label. In this contest, you don't have a good enough QA point to surpass the grade-b threshold. You have a total of 3 Low issues, including this report, so you have 15 points in my evaluation (5 points for low, 1 point for R/NC, 0 for info). However, the threshold for grade-b is 25 points (adjusted from 60% of the best QA report). Therefore, all your QA issues are marked as grade-c.