the use of block number instead of timestamp will cause problems.

[hats-bug-reporter]

Github username: @ArnieGod
Submission hash (on-chain): 0xed24816d80fe74fb2141567dcaf466be4481730401c0cdc77b99f79334f24cec
Severity: medium severity

Description:

Vulnerability Report

Description

contract VmexToken is ERC20Upgraded, ERC20Permit, ERC20Votes {

as we can see from the snippet above, this contract inherits from ERC20Votes. This is where the problem begins.
https://docs.openzeppelin.com/contracts/4.x/governance

It is sometimes difficult to deal with durations expressed in number of blocks because of inconsistent or unpredictable time between blocks. This is particularly true of some L2 networks where blocks are produced based on blockchain usage. Using number of blocks can also lead to the governance rules being affected by network upgrades that modify the expected time between blocks.

You can see the block.number is not a reliable source of metric to measure time for governance
https://community.optimism.io/docs/developers/build/differences/
In optimism, the block.number refers to l2 block.number and it is depends on the usage of the block unlike in ETH, 12 second per block.

In other layer 2 such as arbitrium, the block.number refers to L1 block number
https://developer.arbitrum.io/time#:~:text=Arbitrum%20blocks%20have%20their%20own,at%20a%20relatively%20steady%20rate

POC

1. voting ends in 10 blocks
2. because block number varies on optimism, voting would be 120 seconds on ethereum mainnet, but could be less or more on optimism or arbitrum.
3. let us assume votes are extremely close. 50/50
4. every second counts here, a user may think there is 5 seconds left to vote when in reality, there is no more time left.
5. unpredictability of block number across chains will hurt the voting process and could result in loss of funds.

impact
timing of important functions will be off because of reasons stated above. Block number is treated differntly across chains.

code snippet

VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/packages/contracts/contracts/dependencies/openzeppelin/contracts/governance/ERC20Votes.sol

Lines 43 to 55 in fb396a3

	function clock() public view virtual override returns (uint48) {
	return SafeCast.toUint48(block.number);
	}
	/**
	* @dev Description of the clock
	*/
	// solhint-disable-next-line func-name-mixedcase
	function CLOCK_MODE() public view virtual override returns (string memory) {
	// Check that the clock was not modified
	require(clock() == block.number, "ERC20Votes: broken clock mode");
	return "mode=blocknumber&from=default";
	}

recommended fix
override clock and clock mode using timestamp

 function clock() public view override returns (uint48) {
       return uint48(block.timestamp);
   }

   // solhint-disable-next-line func-name-mixedcase
   function CLOCK_MODE() public pure override returns (string memory) {
       return "mode=timestamp";
   }

[skyao2002]

Thanks for the submission and the proof of concept. Protocols like Aave only allow voting on mainnet with their governance token. We plan to follow this metric and have do not currently plan to allow voting on optimism. This issue does not apply to our protocol.

[skyao2002]

Furthermore, the proof of concept outlines a situation where voters try to vote within the last couple of seconds on an issue that is perfectly split. If they wait for the last couple of seconds, network delays/internet speed are also an issue, so the minor inconveniences of using the block number vs the timestamp does not matter than much.
Also, if a governance proposal has an option that can result in the loss of funds, we have a bigger problem to deal with.