search

code-423n4 / 2024-02-wise-lending-findings

7 stars 5 forks source link

Incorrect TWAP implementation can result in assets being frozen prematurely #36

Closed c4-bot-7 closed 3 months ago

c4-bot-7 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WiseOracleHub/OracleHelper.sol#L494-L495

Vulnerability details

Summary & Impact

The protocol uses Chainlink as the primary source of prices. On top of this, TWAP is used as a sanity check such that when price deviates by more then 2.5% between the two, the asset is frozen. This deviation is re-evaluated after 30 minutes and the process repeats.
Due the incorrect maths while implementing TWAP, currently the assets can get frozen even when the deviation is less than 2.5% or conversely, escape being frozen even though the deviation has breached 2.5%.

Vulnerability Details

TWAPs or Time Weighted Average Prices are by their very definition, "averages". Unlike spot prices, two TWAPS -

Also check out bug raised by OpenZeppelin.

The protocol however multiplies two TWAPs at the following places for conversion of prices into ETH<->USD terms, instead of directly using the correct feeds or making use of spot prices -

Click to see relevant code snippets ```js File: contracts/WiseOracleHub/OracleHelper.sol 460: function _getTwapDerivatePrice( 461: address _tokenAddress, 462: UniTwapPoolInfo memory _uniTwapPoolInfo 463: ) 464: internal 465: view 466: returns (uint256) 467: { 468: DerivativePartnerInfo memory partnerInfo = derivativePartnerTwap[ 469: _tokenAddress 470: ]; 471: 472: uint256 firstQuote = OracleLibrary.getQuoteAtTick( 473: _getAverageTick( 474: _uniTwapPoolInfo.oracle 475: ), 476: _getOneUnit( 477: partnerInfo.partnerTokenAddress 478: ), 479: partnerInfo.partnerTokenAddress, 480: WETH_ADDRESS 481: ); 482: 483: uint256 secondQuote = OracleLibrary.getQuoteAtTick( 484: _getAverageTick( 485: partnerInfo.partnerOracleAddress 486: ), 487: _getOneUnit( 488: _tokenAddress 489: ), 490: _tokenAddress, 491: partnerInfo.partnerTokenAddress 492: ); 493: 494: @---> return firstQuote 495: @---> * secondQuote 496: / uint256( 497: _getOneUnit( 498: partnerInfo.partnerTokenAddress 499: ) 500: ); 501: } ``` ```js File: contracts/DerivativeOracles/PtOracleDerivative.sol 76: /** 77: * @dev Read function returning latest ETH value for PtToken. 78: * Uses answer from Usd chainLink pricefeed and combines it with 79: * the result from ethInUsd for one token of PtToken. 80: */ 81: function latestAnswer() 82: public 83: view 84: returns (uint256) 85: { 86: ( 87: , 88: int256 answerUsdFeed, 89: , 90: , 91: ) = USD_FEED_ASSET.latestRoundData(); 92: 93: ( 94: , 95: int256 answerEthUsdFeed, 96: , 97: , 98: ) = ETH_FEED_ASSET.latestRoundData(); 99: 100: ( 101: bool increaseCardinalityRequired, 102: , 103: bool oldestObservationSatisfied 104: ) = ORACLE_PENDLE_PT.getOracleState( 105: PENDLE_MARKET_ADDRESS, 106: TWAP_DURATION 107: ); 108: 109: if (increaseCardinalityRequired == true) { 110: revert CardinalityNotSatisfied(); 111: } 112: 113: if (oldestObservationSatisfied == false) { 114: revert OldestObservationNotSatisfied(); 115: } 116: 117: uint256 ptToAssetRate = ORACLE_PENDLE_PT.getPtToAssetRate( 118: PENDLE_MARKET_ADDRESS, 119: TWAP_DURATION 120: ); 121: 122: @---> return uint256(answerUsdFeed) 123: * PRECISION_FACTOR_E18 124: / POW_USD_FEED 125: * POW_ETH_USD_FEED 126: / uint256(answerEthUsdFeed) 127: * ptToAssetRate 128: / PRECISION_FACTOR_E18; 129: } ``` ```js File: contracts/DerivativeOracles/PtOraclePure.sol 62: /** 63: * @dev Read function returning latest ETH value for PtToken. 64: * Uses answer from Usd chainLink pricefeed and combines it with 65: * the result from ethInUsd for one token of PtToken. 66: */ 67: 68: function latestAnswer() 69: public 70: view 71: returns (uint256) 72: { 73: ( 74: , 75: int256 answerFeed, 76: , 77: , 78: 79: ) = FEED_ASSET.latestRoundData(); 80: 81: ( 82: bool increaseCardinalityRequired, 83: , 84: bool oldestObservationSatisfied 85: ) = ORACLE_PENDLE_PT.getOracleState( 86: PENDLE_MARKET_ADDRESS, 87: DURATION 88: ); 89: 90: if (increaseCardinalityRequired == true) { 91: revert CardinalityNotSatisfied(); 92: } 93: 94: if (oldestObservationSatisfied == false) { 95: revert OldestObservationNotSatisfied(); 96: } 97: 98: uint256 ptToAssetRate = ORACLE_PENDLE_PT.getPtToAssetRate( 99: PENDLE_MARKET_ADDRESS, 100: DURATION 101: ); 102: 103: @---> return uint256(answerFeed) 104: * PRECISION_FACTOR_E18 105: / POW_FEED 106: * ptToAssetRate 107: / PRECISION_FACTOR_E18; 108: } ```


A vulnerability arising from using TWAPs incorrectly (albeit having a different nature & attack vector) led to the 7.8 million dollar Warp Finance hack in 2020 which was explained in detailed in CMichel's blog.

In our context, the divergence between the actual TWAP price and the incorrectly calculated TWAP means the difference between Chainlink and TWAP can be evaluated as 2.5% even when it is not, causing the freezing of assets. Conversely, it can also result in the assets not being frozen even when the difference has reached 2.5% in reality.

Mathematical PoC

One can provide a proof mathematically to show that two TWAPs can not be multiplied to arrive at the resultant TWAP. The actual Uniswap TWAP has cumulative prices stored which are then used for further calculations, but the following example is good enough to highlight the fundamental issue. Let's assume that the following spot prices exist at different timestamps - t tokenX spot price tokenY spot price tokenA spot price tokenX/tokenA tokenA/tokenY
1 x1 y1 a1 xa1 = x1/a1 ay1 = a1/y1
2 x2 y2 a2 xa2 = x2/a2 ay2 = a2/y2
3 x3 y3 a3 xa3 = x3/a3 ay3 = a3/y3
- TWAP: (x1+x2+x3)/3 TWAP: (y1+y2+y3)/3 TWAP: (a1+a2+a3)/3 TWAP: (xa1+xa2+xa3)/3 TWAP: (ay1+ay2+ay3)/3

As per the protocol's current logic: 

TWAP of tokenX/tokenY = TWAP_tokenX/tokenA * TWAP_tokenA/tokenY = (xa1+xa2+xa3)/3 * (ay1+ay2+ay3)/3 = (xa1+xa2+xa3) * (ay1+ay2+ay3) / 9 = (x1/a1 + x2/a2 + x3/a3) * (a1/y1 + a2/y2 + a3/y3) / 9

The actual TWAP however is:

TWAP of tokenX/tokenY = average of spot price ratios at each timestamp = (x1/y1 + x2/y2 + x3/y3) / 3

It's trivial to see from here on that the two expressions are not equal. The more volatile the price of tokenX/tokenY, the more extreme this divergence in correct & incorrect price is.

Tools Used

Manual review

Recommended Mitigation Steps

Assessed type

Math

GalloDaSballo commented 3 months ago

This looks off in the sense that TWAPs are not being multiplied but the resulting quoteAtTick

There are rounding errors in using QuoteAtTick, but this report seems to focus on something else

Worth reviewing but the multiplication of quotes is not in itself, the same as multiplying a TWAP

c4-pre-sort commented 3 months ago

GalloDaSballo marked the issue as sufficient quality report

c4-pre-sort commented 3 months ago

GalloDaSballo marked the issue as primary issue

vm06007 commented 3 months ago

Should be dismissed

c4-judge commented 3 months ago

trust1995 marked the issue as unsatisfactory: Invalid

t0x1cC0de commented 3 months ago

Hey @trust1995, I am not clear about the reason for invalidation here? The only comment I have available to go on is this one from @GalloDaSballo, which is incorrect:

... TWAPs are not being multiplied but the resulting quoteAtTick ...

and

Worth reviewing but the multiplication of quotes is not in itself, the same as multiplying a TWAP

quoteAtTick is not the "spot" quote at a given tick but rather the mean or average quote at the tick calculated over the past X seconds (i.e. time-weighted average price of TWAP). You can refer the natspec of the official Uniswap implementation of observe() function here:

    /// @notice Returns the cumulative tick and liquidity as of each timestamp `secondsAgo` from the current block timestamp
    /// @dev To get a time weighted average tick or liquidity-in-range, you must call this with two values, one representing
    /// the beginning of the period and another for the end of the period. E.g., to get the last hour time-weighted average tick,
    /// you must call it with secondsAgos = [3600, 0].
    /// @dev The time weighted average tick represents the geometric time weighted average price of the pool, in
    /// log base sqrt(1.0001) of token1 / token0. The TickMath library can be used to go from a tick value to a ratio.
    /// @param secondsAgos From how long ago each cumulative tick and liquidity value should be returned
    /// @return tickCumulatives Cumulative tick values as of each `secondsAgos` from the current block timestamp
    /// @return secondsPerLiquidityCumulativeX128s Cumulative seconds per liquidity-in-range value as of each `secondsAgos` from the current block
    /// timestamp
    function observe(uint32[] calldata secondsAgos)
        external
        view
        returns (int56[] memory tickCumulatives, uint160[] memory secondsPerLiquidityCumulativeX128s);

As a result, we can't just multiply two averages to figure out the third TWAP, as detailed in my report.

In fact, within the protocol itself, the function names themselves are quite clear that the quotes are 'averages':

File: contracts/WiseOracleHub/OracleHelper.sol

472:        uint256 firstQuote = OracleLibrary.getQuoteAtTick(
473:            _getAverageTick(            <--------------------------------- This in turn calls `IUniswapV3Pool(_oracle).observe` on L423 the reference to which I provided above
474:                _uniTwapPoolInfo.oracle
475:            ),

It's also quite obvious on L431 that two cumulatives are being subtracted to figure out the cumulative average price within a specific window. Hence I am not able to make out why this is invalid... Could you please confirm?

Thanks

trust1995 commented 3 months ago

Hi,

You haven't proved multiplying two quotes, fetched from a TWAP is an issue. The provided Fei finding is not relevant.