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

Hardcoded POLL_DURATION_BLOCKS causes lending term offboarding polls' lifetime to be too short for voting on several chains #1131

Closed
c4-bot-8 opened this issue Dec 28, 2023 · 9 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-816 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-8
Copy link
Contributor

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/LendingTermOffboarding.sol#L94-L97
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L120-L123

Vulnerability details

The Ethereum Credit Guild protocol is anticipated to launch on Ethereum Mainnet & L2s like Arbitrum, Optimism, etc.

In the LendingTermOffboarding contract, the POLL_DURATION_BLOCKS constant defines the lifetime of all lending term offboarding polls. The contract hardcoded the value of the POLL_DURATION_BLOCKS to 46523, which is ~7 days on Ethereum Mainnet (at 13s / block).

However, other chains have different block times. For instance, 2s for Optimism/Polygon/Avalanche/Base, 3s for BNB Chain, 1.13s for Fantom, etc. Therefore, the lending term offboarding polls will stay open for ~1 day on Optimism/Polygon/Avalanche/Base, ~1.6 days on BNB Chain, and ~14.6 hours on Fantom.

Consequently, the vote for offboarding terms on the above chains may not succeed as expected since the lifetime of the polls is too short for voting, causing damage to the protocol and users.

Proof of Concept

The LendingTermOffboarding contract fixed the POLL_DURATION_BLOCKS constant to 46523. The POLL_DURATION_BLOCKS determines the lifetime of all lending term offboarding polls in the proposeOffboard() and supportOffboard(), as shown below.

    /// @notice maximum age of polls for them to be considered valid.
    /// This offboarding mechanism is meant to be used in a reactive fashion, and
    /// polls should not stay open for a long time.
@1  uint256 public constant POLL_DURATION_BLOCKS = 46523; // ~7 days @ 13s/block

    ...

    function proposeOffboard(address term) external whenNotPaused {
        require(
            polls[block.number][term] == 0,
            "LendingTermOffboarding: poll exists"
        );
@2      require(
@2          block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS,
@2          "LendingTermOffboarding: poll active"
@2      );
        
        ...
    }

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

        ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Do not use the hardcoded POLL_DURATION_BLOCKS. It should be an immutable variable configurable by a contract deployer via the constructor().

It is important to note that on Arbitrum, the block.number will be updated to sync with Ethereum Mainnet's block.number approximately every minute. Thus, the Arbitrum's block.number will be similar to the Ethereum Mainnet's block.number over time. In other words, the POLL_DURATION_BLOCKS for Arbitrum could be the same as the Ethereum Mainnet.

Assessed type

Other

@c4-bot-8 c4-bot-8 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 28, 2023
c4-bot-8 added a commit that referenced this issue Dec 28, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added sufficient quality report This report is of sufficient quality primary issue Highest quality submission among a set of duplicates labels Dec 29, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as primary issue

@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #1012

@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #816

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jan 24, 2024
@c4-judge
Copy link
Contributor

Trumpero changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-b

@c4-judge
Copy link
Contributor

Trumpero marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed grade-b labels Jan 31, 2024
@serial-coder
Copy link
Member

Hi @Trumpero,

Whereas #816 (primary issue) got a grade-b (eligible for receiving awards), why was this issue rated a grade-c/unsatisfactory(ineligible for receiving awards)?

This issue refers to the incorrect POLL_DURATION_BLOCKS constant only, while #816 refers to the incorrect POLL_DURATION_BLOCKS and a constant in the GuildVetoGovernor::votingPeriod().

Please let me clarify why mentioning the constant in the votingPeriod() of #816 is incorrect.

    @>  /// @dev voting period is unused, it is a duration in blocks for the vote
    @>  /// but the timestamp of the action in the timelock is used to know if the
    @>  /// vote period is over (after action is ready in the timelock, the veto
    @>  /// vote failed).
        function votingPeriod() public pure override returns (uint256) {
            return 2425847; // ~1 year with 1 block every 13s
        }

The GuildVetoGovernor::votingPeriod() was implemented but never used as commented by the developer, which is the correct implementation (after my investigation).

Since the Governor::state() was overridden by the GuildVetoGovernor::state(), the veto vote will be active as long as the proposal/action is pending in the timelock.

For this reason, #816 is partially valid, while this issue is more accurate.

@Trumpero
Copy link

Trumpero commented Feb 7, 2024

@serial-coder When an issue is downgraded to QA, it will be included in the QA report of that warden, then all QA issues of that warden will be label a same grade based on their QA points. After combining all QA issues of this warden, their QA point is still not enough to reach grade-b in my evaluation. Therefore, all issues of this warden are marked as grade-c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-816 grade-c QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

5 participants