KIBToken: `setEpochLength` & `refreshYield` will revert until first epoch

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-kuma/blob/main/src/kuma-protocol/KIBToken.sol#L353 https://github.com/code-423n4/2023-02-kuma/blob/main/src/kuma-protocol/KIBToken.sol#L302

Vulnerability details

Impact

The KIBToken.initialize function sets the _lastRefresh parameter to the next epoch timestamp into the future. While the _calculateCumulativeYield calculates the timeElapsed value based upon block.timestamp and _lastRefresh.

    function initialize(
        ...
    ) external override initializer {
        // ...
        _epochLength = epochLength;
        _lastRefresh = block.timestamp % epochLength == 0
            ? block.timestamp
            : (block.timestamp / epochLength) * epochLength + epochLength;
        // ...
    }

    function _calculateCumulativeYield() private view returns (uint256) {
        uint256 timeElapsed = block.timestamp - _lastRefresh;
        if (timeElapsed == 0) return _cumulativeYield;
        return _yield.rayPow(timeElapsed).rayMul(_cumulativeYield);
    }

Since in initialize the _lastRefresh value is set into the future, until that timestamp is passed the first statement of _calculateCumulativeYield will always get reverted due to integer underflow error.

The _calculateCumulativeYield function is internally invoked in various functions like:

• _refreshCumulativeYield
• setEpochLength
• refreshYield
• mint
• burn
• _transfer

Until the _lastRefresh timestamp is passed all these function invocations will get reverted. While most reverts seem intended, there are cases in which this forceful revert will cause issues:

• setEpochLength In the case where the KUMA_SET_EPOCH_LENGTH_ROLE wants to increase the _epochLength his transaction will always get reverted. Hence preventing the increase of _epochLength.

• refreshYield This is a public function the execution of which will get reverted until the _lastRefresh timestamp is passed. Hence preventing the refreshing of yields.

Proof of Concept

Consider this scenario:

• KIBToken was deployed with 30 days as the epoch length.
• The _lastRefresh was set to a value <30 days into the future.
• After 1 day the KUMA_SET_EPOCH_LENGTH_ROLE decides to update the epoch length to 45 days.
• The setEpochLength call gets reverted due to the underflow error.

In this case, the KUMA_SET_EPOCH_LENGTH_ROLE cannot increase the epoch length until the _lastRefresh timestamp is passed.

Tools Used

Manual review

Recommended Mitigation Steps

Consider initialising the _lastRefresh to a back dated timestamp rather than a future timestamp.

        _lastRefresh = block.timestamp % epochLength == 0
            ? block.timestamp
            : (block.timestamp / epochLength) * epochLength;

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

The finding is correct, but it's circumscribed to an initial time in which the admin has yet to set up the contract

For this reason, I believe this to be a distinct instance, but logically equivalent to a Front-run on Initialize, which we rate QA - Low Severity

L

[c4-judge]

GalloDaSballo marked the issue as grade-c

[GalloDaSballo]

Finding is valid, but after scoring the QA Submissions the score is not sufficient

[akshaysrivastav]

This issue details the scenario in which due to an integer overflow the contract admin is prevented from updating the epoch length for a certain period of time. Essentially a denial of service to admin.

It has nothing to do with contract initialization (and its frontrunning). The explained scenario happens after a complete initial setup was already done by admin.

Can you please review this again @GalloDaSballo @m19

[GalloDaSballo]

Thank you for flagging @akshaysrivastav will double check

[GalloDaSballo]

Have double checked and substantially the issue is:

• You cannot change the setter setEpochLength
• The function refreshYield will revert

until the first epoch

I believe Low Severity to be the most appropriate, a gotcha issue that is valid, but limited in time and impact (only before first epoch, only after initialization)