Looping over unbounded array can result in a state of DoS

[ddimitrov22]

There are unbounded arrays in the codebase that can grow to infinity and result in out-of-gas errors.

In Locker.sol the lockUps array only pushes items when a lockup is created:

function createLockUp(address token, bool isERC20, uint256 amount, uint40 unlockTime, address receiver, string calldata title) external {
        ...
        lockUps.push(); //@audit can grow to infinity
        ...
        emit LockedUp(lockUps.length - 1, token, isERC20, receiver, amount, unlockTime);
    }

If this array grows too big, the getLockUpIdsByToken and the getLockUpIdsByReceiver can exceed the block gas limit and revert because of the for-loop. Additionally, the lockUps are not popped from the array even if the specific lockUp is unlocked. Consider capping the array or popping the lockUps that are unlocked from the array.

The same issue is present in the MerkleDistributor.sol where items are pushed to the distributions array but are never popped.

    function createDistribution(
        ..
    ) external {
     ...
    distributions.push();
     ...
}

Then the getDistributionIdsByOwner and getDistributionIdsByToken functions loop offer the unbounded arrays which can lead to revert with out-of-gas errors.

[the-first-elder]

Yes I already said something like this tho it's a critical issue and likely to happen especially in a big project like this

[sydneyitguy]

Those utility functions

• getDistributionIdsByOwner
• getDistributionIdsByToken
• getLockUpIdsByReceiver

Those are utility functions for off-chain read calls only, and they have pagination parameters (start and stop) for limiting the output size. Is there any case in which those functions can be reverted with a proper start and stop parameter?

@ddimitrov22 @the-first-elder

[ddimitrov22]

There is no validation for the start and stop parameters so any value can be passed. If the size of the array in this period is too big the calls will revert.

[sydneyitguy]

@ddimitrov22 functions can be reverted with an invalid parameter anyway, and this function is used for front-end side utility purposes only. So, I guess there should be no problem.

[ddimitrov22]

Delay will not be a problem as this is not how it works. The only problem is that looping over the entire array can exceed the block gas limit. And passing start and stop values will not result in revert as there is no validation (for example, the start can be quite early and the stop be quite late making the period very long) and will be accepted as valid.

[sydneyitguy]

But this function is a utility read-only function that is intended for off-chain calls, not intended or included in any write calls. If the user calls with invalid parameters like start: 0, stop: 9999 and the call reverts with out-of-gas issue, they can just try with smaller pagination parameters, like start: 0, stop: 100. Is there any problem with this use-case?

[ddimitrov22]

With this specific example - there will be no problem. But it will be impossible to retrieve the whole array ever.

[0x3agle]

Users passing invalid values that cause a loss to themselves are generally considered informational.
What can be done is if the difference between start and stop is huge, the front end could warn the user.

[sydneyitguy]

@ddimitrov22

it will be impossible to retrieve the whole array ever.

This is not true. User can retrieve the whole array by passing a proper pagination parameters like:

1. 1st call - start: 0, stop: 100
2. 2nd call - start: 100, stop: 200
3. 3rd call - start: 200, stop: 300

...

[ddimitrov22]

Yep, it will require several separate calls and keeping track of the results which is not the most convenient way.

[sydneyitguy]

@0x3agle We handle those pagination parameters with our RPC calls, so users don't need to worry about those parameters as well.

[0x3agle]

The issue says that if the array grows too big, these functions will be DoS'ed.
Given there is a functionality to access the array partly, I don't think there's an issue here

[sydneyitguy]

@ddimitrov22 We could use mapping if we don't want to manage an infinitely growing array, but that requires another off-chain storage to make it traversable, or EnumerableMapping on-chain, which incurs an additional gas fee for storing.

[ddimitrov22]

Might be a better solution.

Another issue with only pushing items to the array is that the lockUps array - when a lockUp is unlocked, it will still stay in the array of lockUps as it is not popped from the array. Other functionalities will return wrong results like lockUpCount which will return a value which will include unlocked lockUps.

[sydneyitguy]

We don't use array.pop here. lockUpId is a permanent unique index; we just change its state by setting unlocked to true.

[ddimitrov22]

I meant this one:

    function lockUpCount() external view returns (uint256) {
        return lockUps.length;
    }

It will return the size of the ever-growing array.

[sydneyitguy]

Yes, but as far as I know, array.length is O(1), right?