0x52 - Exponential and logarithmic price adapters will return incorrect pricing when moving from higher dp token to lower dp token

[sherlock-admin2]

0x52

medium

Exponential and logarithmic price adapters will return incorrect pricing when moving from higher dp token to lower dp token

Summary

The exponential and logarithmic price adapters do not work correctly when used with token pricing of different decimal places. This is because the resolution of the underlying expWad and lnWad functions is not fit for tokens that aren't 18 dp.

Vulnerability Detail

AuctionRebalanceModuleV1.sol#L856-L858

function _calculateQuoteAssetQuantity(bool isSellAuction, uint256 _componentQuantity, uint256 _componentPrice) private pure returns (uint256) {
    return isSellAuction ? _componentQuantity.preciseMulCeil(_componentPrice) : _componentQuantity.preciseMul(_componentPrice);
}

The price returned by the adapter is used directly to call _calculateQuoteAssetQuantity which uses preciseMul/preciseMulCeil to convert from component amount to quote amount. Assume we wish to sell 1 WETH for 2,000 USDT. WETH is 18dp while USDT is 6dp giving us the following price:

1e18 * price / 1e18 = 2000e6

Solving for price gives:

price = 2000e6

This establishes that the price must be scaled to:

price dp = 18 - component dp + quote dp

Plugging in our values we see that our scaling of 6 dp makes sense.

BoundedStepwiseExponentialPriceAdapter.sol#L67-L80

    uint256 expExpression = uint256(FixedPointMathLib.expWad(expArgument));

    // Protect against priceChange overflow
    if (scalingFactor > type(uint256).max / expExpression) {
        return _getBoundaryPrice(isDecreasing, maxPrice, minPrice);
    }
    uint256 priceChange = scalingFactor * expExpression - WAD;

    if (isDecreasing) {
        // Protect against price underflow
        if (priceChange > initialPrice) {
            return minPrice;
        }
        return FixedPointMathLib.max(initialPrice - priceChange , minPrice);

Given the pricing code and notably the simple scalingFactor it also means that priceChange must be in the same order of magnitude as the price which in this case is 6 dp. The issue is that on such small scales, both lnWad and expWad do not behave as expected and instead yield a linear behavior. This is problematic as the curve will produce unexpected behaviors under these circumstances selling the tokens at the wrong price. Since both functions are written in assembly it is very difficult to determine exactly what is going on or why this occurs but testing in remix gives the following values:

expWad(1e6) - WAD = 1e6
expWad(5e6) - WAD = 5e6
expWad(10e6) - WAD = 10e6
expWad(1000e6) - WAD = 1000e6

As seen above these value create a perfect linear scaling and don't exhibit any exponential qualities. Given the range of this linearity it means that these adapters can never work when selling from higher to lower dp tokens.

Impact

Exponential and logarithmic pricing is wrong when tokens have mismatched dp

Code Snippet

BoundedStepwiseExponentialPriceAdapter.sol#L28-L88

BoundedStepwiseLogarithmicPriceAdapter.sol#L28-L88

Tool used

Manual Review

Recommendation

scalingFactor should be scaled to 18 dp then applied via preciseMul instead of simple multiplication. This allows lnWad and expWad to execute in 18 dp then be scaled down to the correct dp.

[pblivin0x]

Agree that scalingFactor should be 18 decimals and applied with preciseMul, will fix.

[Oot2k]

Not a duplicate

[bizzyvinci]

Escalate

This is invalid

FixedPointMathLib.expWad and FixedPointMathLib.lnWad uses WAD as input and WAD as output. This is mentioned in docs and you can test it out on remix. Therefore, exp(1) = FixedPointMathLib.expWad(WAD) / WAD and exp(5) = FixedPointMathLib.expWad(5*WAD) / WAD.

expWad(1e6) - WAD = 1e6 is equal to exp(1e-12) - 1 = 1e-12 which is absolutely correct.

For the formula to work, timeCoefficient has to be in WAD. @pblivin0x should look at our DM around this time.

His comment: scalingFactor should be 18 decimals and applied with preciseMul is on a different matter. It's a plan for the future to allow decimal scalingFactor e.g 0.5, 2.5 rather than just integers like 1, 2, 3 etc.

To recap: block.timestamp is in seconds, therefore timeBucket, _timeElapsed and bucketSize are in seconds. _componentPrice, initialPrice, minPrice, maxPrice, priceChange and FixedPointMathLib are in WAD. Therefore, expExpression, expArgument and timeCoefficient has to also be in WAD. scalingFactor is just a scaler unit which the team plan to turn into WAD in the future for more precision with scaling.

[sherlock-admin2]

Escalate

This is invalid

FixedPointMathLib.expWad and FixedPointMathLib.lnWad uses WAD as input and WAD as output. This is mentioned in docs and you can test it out on remix. Therefore, exp(1) = FixedPointMathLib.expWad(WAD) / WAD and exp(5) = FixedPointMathLib.expWad(5*WAD) / WAD.

expWad(1e6) - WAD = 1e6 is equal to exp(1e-12) - 1 = 1e-12 which is absolutely correct.

For the formula to work, timeCoefficient has to be in WAD. @pblivin0x should look at our DM around this time.

His comment: scalingFactor should be 18 decimals and applied with preciseMul is on a different matter. It's a plan for the future to allow decimal scalingFactor e.g 0.5, 2.5 rather than just integers like 1, 2, 3 etc.

To recap: block.timestamp is in seconds, therefore timeBucket, _timeElapsed and bucketSize are in seconds. _componentPrice, initialPrice, minPrice, maxPrice, priceChange and FixedPointMathLib are in WAD. Therefore, expExpression, expArgument and timeCoefficient has to also be in WAD. scalingFactor is just a scaler unit which the team plan to turn into WAD in the future for more precision with scaling.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[IAm0x52]

Since scaling factor is not applied via precise mul in the current implementation, in order to work as the code is written it has to be have the same number of decimals as price and therefore can't be WAD, regardless of what sponsor has said in the discord comments. As I've shown in my issue these smaller values are not compatible with expWAD and lnWAD and will produce incorrect values, negatively affecting auction pricing.

[bizzyvinci]

The getPrice function can be broken down to the following after removing boundary price and type conversions.

1. timeBucket = _timeElapsed / bucketSize
2. expArgument = timeCoefficient * timeBucket
3. expExpression = FixedPointMathLib.expWad(expArgument)
4. priceChange = scalingFactor * expExpression - WAD
5. price = initialPrice + priceChange (or minus)

To know what unit should be WAD or not, we need to look elsewhere.

1. _timeElapsed = block.timestamp - rebalanceInfo[_setToken].rebalanceStartTime here and rebalanceInfo[_setToken].rebalanceStartTime is set to block.timestamp when startRebalance is called here. Therefore _timeElapsed, bucketSize and timeBucket has to be seconds.
2. _componentPrice is in precise unit or WAD based on calculation here. Therefore price, initialPrice and priceChange have to also be in WAD.
3. Formula 4 is wrong as pointed out in #39 therefore let's just focus on the multiplication part and assume priceChange = scalingFactor * expExpression. If priceChange is WAD, then scalingFactor * expExpression has to be WAD. Either scalingFactor is WAD or expExpression is WAD.
4. FixedPointMathLib.expWad returns WAD, so expExpression is indeed WAD. So scalingFactor is basic unit.
5. Furthermore, FixedPointMathLib.expWad takes WAD as input, and that input is timeCoefficient * timeBucket. We've established that timeBucket is seconds in 1, so therefore timeCoefficient has to be WAD.

The sponsor's message was referenced because

• I was the one who decided that his initial statement timeCoefficient and bucketSize are not in WAD is wrong. So he might want to cross-check things again.
• We had some discussions about scalingFactor and converting it to WAD around that time.

If scalingFactor is changed to WAD, then priceChange would be WAD^2. Therefore, we must use preciseMul to keep things balanced again. P.S: -WAD is ignored again.

+ priceChange = scalingFactorWAD.preciseMul(expExpression)
- priceChange = scalingFactorBasic * expExpression

The 2 formula are the same thing because preciseMul(a, b) = a * b / WAD code

function preciseMul(uint256 a, uint256 b) internal pure returns (uint256) {
    return a.mul(b).div(PRECISE_UNIT);
}

[IAm0x52]

_componentPrice is in precise unit or WAD based on calculation here. Therefore price, initialPrice and priceChange have to also be in WAD.

This is incorrect. I've proven in my submission above that when going from an 18 dp token to 6 dp that price has to be 6 dp. Since scalingFactor is applied as a scaler and not via preciseMul then expArgument and expExpression have to also be in 6 dp as well. If you used a WAD expression for them the pricing would be completely wrong as it would return an 18 dp price. As I've shown expWAD returns incorrectly when inputting a 6 dp number.

[IAm0x52]

The point of this issue is to prove that scaling factor must be applied via preciseMul or else the price cannot work as expect. To just say "scaling factor should be applied via preciseMul" is not a valid issue unless you can show why it's incorrect that it's not applied that way and the damages that it causes.

[bizzyvinci]

precise unit is used for precision in calculation because the numbers could be very small and solidity does automatic rounding. When multiplying or dividing, preciseMul or preciseDiv is used to finally get rid of that precision. You can view the PreciseUnitMath library and the key take away are

1. PRECISE_UNIT = PRECISE_UNIT_INT = WAD = 1e18
2. preciseMul(a, b) = a * b / WAD and that only makes sense if a or b is WAD
3. preciseDiv(a, b) = a * WAD / b and that only makes sense if b is WAD

Now, why is _componentPrice in WAD? Because _componentQuantity and the returned quoteAssetQuantity are not in WAD and preciseMul needs b to be WAD. _componentQuantity and quoteAssetQuantity are the raw quantity amount that would be transferred with token.transfer.

https://github.com/sherlock-audit/2023-06-Index/blob/main/index-protocol/contracts/protocol/modules/v1/AuctionRebalanceModuleV1.sol#L856-858

function _calculateQuoteAssetQuantity(bool isSellAuction, uint256 _componentQuantity, uint256 _componentPrice) private pure returns (uint256) {
      return isSellAuction ? _componentQuantity.preciseMulCeil(_componentPrice) : _componentQuantity.preciseMul(_componentPrice);
  }

[pblivin0x]

I believe this is a valid medium

[Oot2k]

I agree that this is valid medium

[bizzyvinci]

I still stand by my escalation and I think my proof is sufficient cause it proves the following

• _componentPrice in WAD
• Right now, scalingFactor is not WAD cause that would be mathematically wrong. It must be a normal integer.
• The unit of each parameters (e.g which one is seconds, WAD or int) to show that price does work correctly.
• The team plans to make scalingFactor WAD and use preciseMul. They must use preciseMul to make sure priceChange remains a WAD (rather than WAD^2)
• The migration of scalingFactor from integer scalar to WAD scalar would not change price nor priceChange because preciseMul(a,b) = a * b / WAD
• The only effect the migration has is precision. scalingFactor could then be represented as decimals instead of just integers.

If anyone disagrees it would be nice if they state why. Or if there's a point that wasn't clear, I'm here to clarify.

[bizzyvinci]

I understand the proof might be daunting to comprehend so I'll recommend using pen and paper (and maybe remix with calculator) to make things easier.

[bizzyvinci]

I do agree that my escalation be rejected

[hrishibhat]

Result: Medium Has duplicates Considering this a valid medium based on the above comments.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• bizzyvinci: rejected

[pblivin0x]

The remediation for this issue is open for review here https://github.com/IndexCoop/index-protocol/pull/25

The changes are to update to 18 decimal scalingFactor in both the exponential and logarithmic adapter

• https://github.com/IndexCoop/index-protocol/blob/839a6c699cc9217c8ee9f3b67418c64e80f0e10d/contracts/protocol/integration/auction-price/BoundedStepwiseExponentialPriceAdapter.sol#L73
• https://github.com/IndexCoop/index-protocol/blob/839a6c699cc9217c8ee9f3b67418c64e80f0e10d/contracts/protocol/integration/auction-price/BoundedStepwiseLogarithmicPriceAdapter.sol#L73

[IAm0x52]

Fix looks good. scalingFactor is now applied via mulWad