search

code-423n4 / 2023-12-ethereumcreditguild-findings

17 stars 11 forks source link

`clock()` will not work properly for Arbitrum due to usage of `block.number` #1240

Closed c4-bot-7 closed 8 months ago

c4-bot-7 commented 9 months ago

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fd81a96f01cc42ef1c9a5399364968d0e07e9e90/contracts/governance/extensions/GovernorVotes.sol#L25

Vulnerability details

Impact

clock() and CLOCK_MODE() will not work correctly on Arbitrum.

This issue is similar to https://github.com/code-423n4/2023-06-lybra-findings/issues/114

Proof of Concept

GuildGovernor, GuildVetoGovernor and LendingTermOnboarding rely internally on the clock() function provided by OZ's GovernorVotes, which returns the block number by default if the voting token doesn't provide a clock() function. Furthermore, block.number is employed in multiple instances in ERC20MultiVotes to determine voting power. However, on Arbitrum, block.number returns the most recently synced L1 block number, which is only updated once per minute. This can lead to inaccurate timing, as the block number does not accurately reflect the passage of time on these networks.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fd81a96f01cc42ef1c9a5399364968d0e07e9e90/contracts/governance/extensions/GovernorVotes.sol#L25-L43 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fd81a96f01cc42ef1c9a5399364968d0e07e9e90/contracts/governance/Governor.sol#L180 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fd81a96f01cc42ef1c9a5399364968d0e07e9e90/contracts/governance/Governor.sol#L277 ^ the above functions are called internally through super. calls, either directly or through GovernorTimelockControl

Tools Used

Manual review

Recommended Mitigation Steps

Inherit from IERC6372 in GuildToken and CreditToken and provide a clock() function based on the L2 block number. Additionally, update instances of block.number in ERC20MultiVotes to use the L2 block number.

Assessed type

Timing

c4-pre-sort commented 8 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 8 months ago

0xSorryNotSorry marked the issue as duplicate of #816

c4-judge commented 8 months ago

Trumpero marked the issue as not a duplicate

c4-judge commented 8 months ago

Trumpero changed the severity to QA (Quality Assurance)

Trumpero commented 8 months ago

According to the verdict, this issue is considered as OOS.

c4-judge commented 8 months ago

Trumpero marked the issue as grade-c