Wrong consideration of number of blocks in `blockCountInAWeek`

[0xRizwan]

Issue Title

Wrong consideration of number of blocks in blockCountInAWeek

Issue severity

Medium

Vulnerability details

DiamondInit.init() contract is used to initializes a diamond with user defined state variables. The issue is the number of blocks considered in blockCountInAWeek is incorrect which will break the core intended design of contracts functionalities.

File: src/dollar/upgradeInitializers/DiamondInit.sol

        ls.blockCountInAWeek = 45361;

Affected code location: check here

The number of seconds in one week is 6,04,800 seconds and while calculating the blockCountInAWeek, the blocks formation period is considered to be 13.3 seconds. It is to be noted that Ethereum Proof of Work is deprecated and merged with Proof of stake which has different block formation period. It is explained as below,

1) During of Ethereum, Proof of work 13 to 16 seconds was an average block formation time before Ethereum merge i.e before september, 2022 and 13.3 seconds is considered in current implementation.

**BEFORE merge Ethereum block time reference with chart:

However, Ethereum block formation happens on every 12 seconds and it is confirmed from below sources, Ethereum official Reference-01 chainstack Reference-02 ycharts Reference-03

**AFTER merge Ethereum block time reference with chart:

To see the actual difference,

With current implementation:

Total seconds in one week = 6,04,800
Considered Ethereum block formation period in second in current implementation = 13.33 approx
Therefore, ls.blockCountInAWeek = 45361;

With Ethereum Proof of stake,

Total seconds in one week = 6,04,800
Proposed Ethereum block formation period in second  = 12 approx
Therefore, ls.blockCountInAWeek = 50400;

The difference = 50400 - 45361 = 5039 = 17 hours approx.

This much time difference will affect wherever blockCountInAWeek variable used in contracts which will cause unexpected design failure.

In LibStaking.sol, In deposit() function, To calculate the _endBlock, blockCountInAWeek variable is used,

    function deposit(
        uint256 _lpsAmount,
        uint256 _weeks
    ) internal returns (uint256 _id) {

    . . . some code

        // calculate end locking period block number
        uint256 _endBlock = block.number + _weeks * ss.blockCountInAWeek;

    . . . some code

Similarly, in functions like addLiquidity(),

    function addLiquidity(
        uint256 _amount,
        uint256 _id,
        uint256 _weeks
    ) internal {

    . . . some code

        stake.endBlock = block.number + _weeks * ss.blockCountInAWeek;

    . . . some code

The end block calculation is getting delayed by 17 hours approx. which is loss of time of users in both of these functions due to incorrect calculations.

Recommended Mitigation Steps

Consider 12 seconds block formation period.

-        ls.blockCountInAWeek = 45361;
+        ls.blockCountInAWeek = 50400;

cc- @rndquu , @pavlovcik , @molecula451 , @gitcoindev

[0x4007]

Thanks for the thorough research on this.

However we already have setters to address this type of issue. This is because blocks may not consistently be 12 seconds forever (as you note before and after the merge.)

I'm not sure exactly how issue severity is ranked, but given that we have setters to handle situations like this, I would consider this a valid, but very low severity issue.

https://github.com/search?q=repo%3Aubiquity%2Fubiquity-dollar+function+setBlockCountInAWeek&type=code

[0xRizwan]

Hi @pavlovcik,

Thanks for the comment.

The issue was identified as Medium severity since it's breaking the protocol intended design. Since the information is known to protocol team and this can be adjusted periodically as Ethereum changes the block formation period in future.

Thank you for confirming the issue and I think, the issue can be considered as low severity as there is setter function as referenced by you and it can be used to adjust number of blocks over time.

[ubiquibot[bot]]

# Issue was not closed as completed. Skipping.