search

sherlock-audit / 2023-06-Index-judging

6 stars 2 forks source link

qpzm - Overflow check is imperfect in `BoundedStepwiseLogarithmicPriceAdapter` and `BoundedStepwiseExponentialPriceAdapter`. #66

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

qpzm

medium

Overflow check is imperfect in BoundedStepwiseLogarithmicPriceAdapter and BoundedStepwiseExponentialPriceAdapter.

Summary

Overflow check is imperfect in BoundedStepwiseLogarithmicPriceAdapter and BoundedStepwiseExponentialPriceAdapter.

Vulnerability Detail

Add this describe block in https://github.com/sherlock-audit/2023-06-Index/blob/ef6f395539970e7c70497e895594b2dcbd2f8344/index-protocol/test/protocol/integration/auction-price/boundedStepwiseExponentialPriceAdapter.spec.ts#L104, and run yarn test.

describe("when it is not decreasing but timeBucket become too big", async () => {
  beforeEach(async () => {
    subjectIsDecreasing = false;
    subjectMaxPrice = ether(110);
    subjectMinPrice = ether(100);
    subjectIncreaseTime = constants.MaxInt256.div(subjectExponent).mul(subjectBucketSize).add(1000000);
    console.log(subjectIncreaseTime);
    subjectPriceAdapterConfigData = await boundedStepwiseExponentialPriceAdapter.getEncodedData(
      subjectInitialPrice,
      subjectCoefficient,
      subjectExponent,
      subjectBucketSize,
      subjectIsDecreasing,
      subjectMaxPrice,
      subjectMinPrice
    );
  });

  it.only("should return the correct price", async () => {
    await subject();
  });

  afterEach(async () => {
    subjectIsDecreasing = true;
    subjectMaxPrice = ether(100);
    subjectMinPrice = ether(90);
    subjectIncreaseTime = ONE_HOUR_IN_SECONDS;
    subjectPriceAdapterConfigData = await boundedStepwiseExponentialPriceAdapter.getEncodedData(
      subjectInitialPrice,
      subjectCoefficient,
      subjectExponent,
      subjectBucketSize,
      subjectIsDecreasing,
      subjectMaxPrice,
      subjectMinPrice
    );
  });
});

I added hardhat console.log to show the overflow. https://github.com/sherlock-audit/2023-06-Index/blob/ef6f395539970e7c70497e895594b2dcbd2f8344/index-protocol/contracts/protocol/integration/auction-price/BoundedStepwiseExponentialPriceAdapter.sol#L58-L67.

    if (timeBucket > type(uint256).max / timeCoefficient) {
        return _getBoundaryPrice(isDecreasing, maxPrice, minPrice);
    }
    int256 expArgument = int256(timeCoefficient * timeBucket);
    console.logInt(expArgument); // -57896044618658097711785492504343953926634992332820282019452584007913129639936

    // Protect against exponential overflow and increasing relative error
    if (expArgument > MAX_EXP_ARG) {
        return _getBoundaryPrice(isDecreasing, maxPrice, minPrice);
    }
    uint256 expExpression = uint256(FixedPointMathLib.expWad(expArgument));
    console.log(expExpression); // 0

In yarn chain terminal, console.log is printed.

eth_call
  Contract call:       BoundedStepwiseExponentialPriceAdapter#getPrice
  From:                0x5409ed021d9299bf6814279a6a1411a7e866a631
  To:                  0xc1486de02ef0d31da8e3fc29b64798dab327e7c2

  console.log:
    -57896044618658097711785492504343953926634992332820282019452584007913129639936
    0

  Error: VM Exception while processing transaction: reverted with panic code 0x12 (Division or modulo division by zero)
      at BoundedStepwiseExponentialPriceAdapter.getPrice (contracts/protocol/integration/auction-price/BoundedStepwiseExponentialPriceAdapter.sol:73)

expExpression returns 0 because FixedPointMathLib.expWad returns 0 if the input is too little. This causes the next line scalingFactor > type(uint256).max / expExpression to revert with a division by zero panic code. Reference: https://github.com/Vectorized/solady/blob/main/src/utils/FixedPointMathLib.sol#L125

Impact

In two specified price adapters, getPrice() will revert if type(int256).max < _timeElapsed / bucketSize * timeCoefficient < type(uint256).max holds. This makes an auction stop.

Code Snippet

Tool used

Manual Review

Recommendation

Check overflow in int256 type.

- if (timeBucket > type(uint256).max / timeCoefficient) {
+ if (timeBucket > uint256(type(int256).max) / timeCoefficient) {

Duplicate of #1