`LiquidInfrastructureERC20::_isPastMinDistributionPeriod()` - L220: should use `>` and not `>=`.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L220 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L181-L182 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L268-L269 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L290-L291 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L303-L304

Vulnerability details

It's clear from the definition of the state variable MinDistributionPeriod that it "Holds the minimum number of blocks required to elapse before a new distribution can begin", which clearly translates to > and not >= for the following check on L220:

return (block.number - LastDistribution) >= MinDistributionPeriod;

To boost clarity in understanding this: A minimum number of blocks need to ELAPSE before a new distribution can begin. The key here is ELAPSED. This clearly means that >= is incorrect because it doesn't exclude the last block from being used for the new distribution, where > would ensure that the new distribution can only start AFTER the last block of the previous distribution period.

Affected areas/functions of the contract: https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L181-L182 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L268-L269 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L290-L291 https://github.com/code-423n4/2024-02-althea-liquid-infrastructure/blob/3adc34600561077ad4834ee9621060afd9026f06/liquid-infrastructure/contracts/LiquidInfrastructureERC20.sol#L303-L304

IMPACT:

I deem this to be at least medium severity because of the below impacts which violates some important invariants.

The problem with the above is two things:

• can begin new distribution AT the last block of the minimum distribution period, which is not permitted according to protocol logic.
• new distribution period's start block will overlap with previous distribution's last block
• mint, burn, transfer of tokens can happen during last block of minimum distribution period

The above is exactly what >= allows but > prevents...

Unless there is a specific valid reason for using >=, should use > instead.

A related issue with the protocol's existing hardhat tests is mentioned below in the PoC section.

PoC:

The earliest it should be possible/allowed to start the next/new distribution is at: block.number > LastDistribution + MinDistributionPeriod Because lets say: LastDistribution = 1000 MinDistributionPeriod = 500 Then: LastDistribution + MinDistributionPeriod = 1500, and block 1500 here is CLEARLY the last block of MinDistributionPeriod, which is not a valid block for the next distribution period to start, because it is still part of the last block of current distribution period. So the next/earliest valid start block for new distribution period is exactly LastDistribution + MinDistributionPeriod + 1 block.

Note: the above "bug" is self-explanatory in terms of logic. The below is an explanation of what happens during protocol tests. Wanted to use existing tests just to "prove" my argument, but instead the tests didn't behave as expected. See below.

Running existing protocol tests, i.e. liquid-infrastructure/test/liquidERC20.ts, were not very helpful as I found that the tests still passed for values less than mine(500), when the MinDistributionPeriod was set to 500. They should not have passed.

Specifically for values 495-499(mined blocks), it sometimes failed, but mostly didnt. I assume this could be due to time delays or randomizing behaviour in terms of the tests, however this is just my assumption. So unless the protocol is already aware of this, their tests do NOT fail 100% of the time for every test run when the MinDistributionPeriod has not passed yet. This is a problem?

I plan to write some foundry tests to check this further.

Here's a test result using mine(495) instead of mine(500), and the run before this one was successful, demonstrating inconsistencies in terms of the tests with regards to at least the MinDistributionPeriod checks behaviour. I modified lines L337 and L402 in test liquidERC20.ts:

  LiquidInfrastructureERC20 tests
Manage
Bad Signer
Not NFT Owner
    ✔ manages NFTs (2071ms)
    ✔ adds and releases NFTs (553ms)
    ✔ manages holders (592ms)
    ✔ manages distributions (basic) (567ms)
revenue: [
  4742536088090499165978624n,
  2867501050966133576826880n,
  11285325741715671614488576n,
  327574195019602099961856n,
  777062924208093542744064n
], distributions: 5
    1) manages distributions (random)

  LiquidInfrastructureNFT tests
Owner tests
Approval tests
    ✔ works right (977ms)

  5 passing (9s)
  1 failing

  1) LiquidInfrastructureERC20 tests
       manages distributions (random):
     Error: VM Exception while processing transaction: reverted with reason string 'MinDistributionPeriod not met'

Recommendation:

    function _isPastMinDistributionPeriod() internal view returns (bool) {
        // Do not force a distribution with no holders or supply
        if (totalSupply() == 0 || holders.length == 0) {
            return false;
        }

-       return (block.number - LastDistribution) >= MinDistributionPeriod;
+       return (block.number - LastDistribution) > MinDistributionPeriod;
    }

Assessed type

Other

[0xRobocop]

Consider QA at best, it does not describe any impact.

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Overinflated severity