c4-bot-1 commented 6 months ago

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L85-L148

Vulnerability details

Proof of Concept

From LendingTermOffboarding.sol#L36)

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

Note that this value is used when supporting or proposing an offboard as can be seen with this search command: https://github.com/search?q=repo%3Acode-423n4%2F2023-12-ethereumcreditguild%20POLL_DURATION_BLOCKS&type=code, i.e LendingTermOffboarding.sol#L85-L148

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

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        require(
            //@audit
            block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll expired"
        );
//...ommited for brevity
    }

As seen both functions check the block.number to be in respect of the intended design with POLL_DURATION_BLOCKS, i.e > lastPollBlock[term] + POLL_DURATION_BLOCKS in regards to proposing and <= snapshotBlock + POLL_DURATION_BLOCKS in regards to supporting, issue is that this should be 7 days but protocol makes the assumption that 1 block is mined every 13s, now this makes protocol off over a month on the ethereum mainnet, this is cause 46523 amounts to less than 6,5 days which is around 13 hours off the intended value.

Also protocol is actually going to be deployed not only on the Ethereum mainnet but even other chains as clearly stated here, what this now leads to is having a way off number for the voting period depending on what chain this gets deployed, for example the official docs of Arbitrum, clearly states that a single Ethereum block includes multiple Arbitrum blocks within it which leads to this value been shorter than necessary in this case, and if takes it shorter on a different chain that protocol gets deployed on then the same case would be made(possibly worse)

Impact

Protocol wrongly implements the intended logic of proposing/supporting offboards, since the stipulated time frame would be way off than is expected/documented... multiple cases coud arise with this, from expiration of snapshots being earlier, or in some cases in regards to supportOffboard() or lasting over 7 days.

Recommended Mitigation Steps

Do not hardcode the POLL_DURATION_BLOCKS value for all chains and also it shouldn't be 46523 for the Ethereum mainnet as this is over 13 hours off than the original value.

Additional Note

This also seems to subtly breaks the logic in contrast to LendingTermOnboarding.sol cause as stated in LendingTermOnboarding.sol#L25, the minimum delay between proposals should be exactly 7 days, cause from LendingTermOnboarding#proposeOnboard() we can see that there is a connection between off boarding and then re-onboarding a particular lending term, i.e terms that have been offboarded in the past can be re-onboarded.

Lastly, a similar bug case seems to be present with the implementation of GuildVetoGovernor.votingPeriod(), since protcol still wrongly hardcodes the duration it takes to mine a block which might lead to protocol to incorrectly relay the voting period.

Assessed type

Timing

c4-pre-sort commented 5 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 5 months ago

0xSorryNotSorry marked the issue as duplicate of #816

c4-judge commented 5 months ago

Trumpero changed the severity to QA (Quality Assurance)