KIBToken: Unexpected reverts of `_calculateCumulativeYield` & `_calculatePreviousEpochCumulativeYield`

[code423n4]

Lines of code

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

Vulnerability details

Impact

In KIBToken the _calculateCumulativeYield & _calculatePreviousEpochCumulativeYield functions intend to calculate the yields using the formula _yield.rayPow(time).rayMul(_cumulativeYield).

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

    function _calculatePreviousEpochCumulativeYield() private view returns (uint256) {
        uint256 previousEpochTimestamp = _getPreviousEpochTimestamp();
        if (previousEpochTimestamp < _lastRefresh) {
            return _previousEpochCumulativeYield;
        }
        uint256 timeElapsedToEpoch = previousEpochTimestamp - _lastRefresh;
        return _yield.rayPow(timeElapsedToEpoch).rayMul(_cumulativeYield);
    }

The _yield and _cumulativeYield variables are initialized with 1e27 value. These values can also increase with time.

As the time elapsed value also increases, the statement _yield.rayPow(time).rayMul(_cumulativeYield) will soon start getting reverted due to Solidity's integer overflow error.

This will lead to reverting of all the functions which invoke the _calculateCumulativeYield & _calculatePreviousEpochCumulativeYield. Hence resulting in DoS for the contract.

Proof of Concept

Consider this scenario:

• The _yield and _cumulativeYield variables are set as 1.1e27.
• As the value of elapsed time increases, the _yield.rayPow(time).rayMul(_cumulativeYield) statement will start getting reverted.

Tools Used

Manual review

Recommended Mitigation Steps

Consider re-thinking about the formula again so that any overflow can be prevented. Using slightly scaled down values instead of 1e27 for _yield and _cumulativeYield variables can also be a solution.

[GalloDaSballo]

Looks off https://github.com/code-423n4/2023-02-kuma/blob/22fd56b3f0df71714cb71f1ce2585f1c4dd21d64/src/kuma-protocol/libraries/WadRayMath.sol#L127

[GalloDaSballo]

Because math is unchecked we would expect overflow / underflows at worse, but these need to be demonstrated

[GalloDaSballo]

Disputing due to this test I wrote:

// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.17;

import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
import {WadRayMath} from "@kuma/libraries/WadRayMath.sol";

import {Test, stdStorage, console2} from "forge-std/Test.sol";

contract OverflowRevert is Test {
  using WadRayMath for uint256;
  // Yield = Rate
  // Time = X
  // Cumulative =  RAY

uint256 public constant cumulative = WadRayMath.RAY;
uint256 public yield = WadRayMath.RAY;

  function test_overflow(uint40 timeElapsed) public {
    yield.rayPow(timeElapsed).rayMul(cumulative);
  }
}

@akshaysrivastav lmk if you have a counter-example that makes sense, I believe the test above is already pretty extreme

[akshaysrivastav]

@GalloDaSballo thanks for asking for more context on this. Your test isn't reverting because both yield and cumulative values you used are 1e27.

But as soon as the those variables are set to any other value like 1.01e27. The computations will start reverting.

See this test case:

// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.17;

import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
import {WadRayMath} from "@kuma/libraries/WadRayMath.sol";

import {Test, stdStorage, console2} from "forge-std/Test.sol";

contract OverflowRevert is Test {
    using WadRayMath for uint256;

    uint256 public constant cumulative = 1.01e27;
    uint256 public yield = 1.01e27;

    function test_no_revert() public {
        uint40 timeElapsed = 5336;
        uint256 result = yield.rayPow(timeElapsed).rayMul(cumulative);
        assertEq(result, 115656987992934942436623407770462120693254366719006);
    }
    function test_revert() public {
        uint40 timeElapsed = 5337;
        vm.expectRevert();
        yield.rayPow(timeElapsed).rayMul(cumulative);
    }
    function test_fuzz_revert(uint40 timeElapsed) public {
        vm.assume(timeElapsed > 5336);
        vm.expectRevert();
        yield.rayPow(timeElapsed).rayMul(cumulative);
    }
}

In the above scenarios 1.01e27 was used as yield and cumulative, and as soon as the timeElapsed value goes over 5336 (~89 minutes) the computations start reverting.

The reverts can start happening sooner in case the yield and cumulative variables are set as higher values.

[c4-sponsor]

m19 marked the issue as disagree with severity

[m19]

We disagree with the severity of this one. While technically true as demonstrated, we feel these values are not at all realistic. Considering bonds yield single digits per year right now and even our aggregator has a max of 30% per year and even in that extreme it only becomes a problem after 500 years.

[GalloDaSballo]

After some consideration, I believe the most appropriate action is to close the report as technically incorrect

Recommend the Warden to follow up with the Sponsor if they can find a specific instance of overflow

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid