Block time is not the same on different chains

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L36
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L116
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20MultiVotes.sol#L374
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L95

Vulnerability details

Summary

Block time refers to the time a block is mined in. The average block time in Ethereum is 12s, but this value is different on different chains.
hardcoded time values dependent on the block.number could break major things on other chains.
As protocol has mentioned in the contest readme we anticipate launching on Ethereum mainnet & L2s like Arbitrum

Impact

All hardcoded timePeriod will not work correctly on Arbitrum as expected

Proof of Concept

block.number is NOT a reliable source of timing information for short-terms
On Arbitrum it reflects the L1 block number, which is updated once per minute even according the doc this can be updated multiple times per L1 block (this lets the sequencer give sub-L1-block-time transaction receipts.)
Once per minute, the block number in the Sequencer is synced to the actual L1 block number. This period could be abused to do many malicious actions
eg from the Governance where a user can vote even deadline is passed
All block.number based calculations can be exploited in multiple ways i.e( H/M/L)

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        require(
            block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll expired"
        );
        uint256 _weight = polls[snapshotBlock][term];
        require(_weight != 0, "LendingTermOffboarding: poll not found");
        uint256 userWeight = GuildToken(guildToken).getPastVotes(
            msg.sender,
            snapshotBlock
        );
        require(userWeight != 0, "LendingTermOffboarding: zero weight");
        require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

        userPollVotes[msg.sender][snapshotBlock][term] = userWeight;
        polls[snapshotBlock][term] = _weight + userWeight;
        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }
        emit OffboardSupport(
            block.timestamp,
            term,
            snapshotBlock,
            msg.sender,
            userWeight
        );
    }

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L36
Here block.number<=snapshotBlock + POLL_DURATION_BLOCKS can be bypassed easily due to the different behaviour of block.number in arbitrum i.e A user could vote 1 arbitrum block before the sync happens and close it the very next block. It would appear that there have been 5 blocks since the last transaction but in reality, it has only been 1 Arbitrum block.
Also, Arbitrum has 2 seconds blocks It would be impossible to block this behavior through param changes

In many places time is used based on the number of blocks which will also create critical issues like manipulating the auction mechanism, voting time period etc

Tools Used

Manual

Recommended Mitigation Steps

block.number is not a trusted source of time instead it's better to use block.timestamps in all such places

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1012

[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

[viraj124]

hey @Trumpero
could you please re-evaluate?

Thanks

[Trumpero]

@viraj124 Replied here

[viraj124]

Okay thanks, @Trumpero
could you remove the unsatisfactory tag so it is considered as a QA