Possible DoS when retrieving the next epoch due to a out of bounds loop

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L438-L451 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L430-L432 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L307-L322

Vulnerability details

Impact

In Vault.sol, if the epochs array grows very large over time, calling getNextEpoch() might consume more gas than the block limit.

Proof of Concept

getNextEpoch will loop through epochs.length

function getNextEpoch(uint256 _epoch)
    public
    view
    returns (uint256 nextEpochEnd)
{
    for (uint256 i = 0; i < epochsLength(); i++) {
        if (epochs[i] == _epoch) {
            if (i == epochsLength() - 1) {
                return 0;
            }
            return epochs[i + 1];
        }
    }
}

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L438-L451

function epochsLength() public view returns (uint256) {
    return epochs.length;
}

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L430-L432

For each createAssets() call, a new epoch will be pushed into the array

function createAssets(uint256 epochBegin, uint256 epochEnd, uint256 _withdrawalFee)
    public
    onlyFactory
{
    ...
    epochs.push(epochEnd);

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L307-L322

Recommended Mitigation Steps

There's three possible solutions

1. Prevent the epochs array from getting too large
2. If epochs is garanteed to be sorted, it's possible to use binary serch instead of linear search to retrieve the next epoch in getNextEpoch()
3. Accept optional arguments startIndex and a endIndex to avoid having to iterate all the items in getNextEpoch(). e.g. a frontend could call this function with different start and end indexes only if calling from 0 to length - 1 results in an out-of-gas error.

[MiguelBits]

not a bad idea, I will just reverse the logic and make the for loop start from the end of the array

[HickupHH3]

I agree with the issue; but because it's a view function that isn't called internally nor by other contracts in scope, there is no impact on funds.

Hence, the issue should be QA (also because it would take quite a number of epochs before gas limit usage is exceeded).

[HickupHH3]

part of #463