TradingLibrary.verifyPrice is using a deprecated ChainLink API, which can lead to a stale price being returned.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/TradingLibrary.sol#L113

Vulnerability details

Impact

TradingLibrary.verifyPrice is using the Chainlink latestAnswer function to retrieve an asset price. This function is deprecated, and does not implement any round check to ensure the price is not stale.

This means verifyPrice might have _priceData.price pass the checks with a stale price, ultimately leading to unhealthy trades opened or closed in Trading.

Proof of Concept

The library function verifyPrice calls the ChainLink API:

112:         if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
113:             int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
114:             if (assetChainlinkPriceInt != 0) {
115:                 uint256 assetChainlinkPrice = uint256(assetChainlinkPriceInt) * 10**(18 - IPrice(_chainlinkFeed).decimals());
116:                 require(
117:                     _priceData.price < assetChainlinkPrice+assetChainlinkPrice*2/100 &&
118:                     _priceData.price > assetChainlinkPrice-assetChainlinkPrice*2/100, "!chainlinkPrice"
119:                 );
120:             }
121:         }

The library function is called by TradingExtension.getVerifyPrice:

163: function getVerifiedPrice(
164:         uint _asset,
165:         PriceData calldata _priceData,
166:         bytes calldata _signature,
167:         uint _withSpreadIsLong
168:     ) 
169:         public view
170:         returns(uint256 _price, uint256 _spread) 
171:     {
172:         TradingLibrary.verifyPrice(
173:             validSignatureTimer,
174:             _asset,
175:             chainlinkEnabled,
176:             pairsContract.idToAsset(_asset).chainlinkFeed,
177:             _priceData,
178:             _signature,
179:             isNode
180:         );
181:         _price = _priceData.price;

Which is used to compute the price of assets in all the functions of Trading:

163:     function initiateMarketOrder(
164:         TradeInfo calldata _tradeInfo,
165:         PriceData calldata _priceData,
166:         bytes calldata _signature,
167:         ERC20PermitData calldata _permitData,
168:         address _trader
169:     )
170:         external
171:     {
...
182:         (uint256 _price,) = tradingExtension.getVerifiedPrice(_tradeInfo.asset, _priceData, _signature, _isLong);

222:     function initiateCloseOrder(
223:         uint _id,
224:         uint _percent,
225:         PriceData calldata _priceData,
226:         bytes calldata _signature,
227:         address _stableVault,
228:         address _outputToken,
229:         address _trader
230:     )
231:         external
232:     {
...
239:         (uint256 _price,) = tradingExtension.getVerifiedPrice(_trade.asset, _priceData, _signature, 0);

413:     function removeMargin(
414:         uint256 _id,
415:         address _stableVault,
416:         address _outputToken,
417:         uint256 _removeMargin,
418:         PriceData calldata _priceData,
419:         bytes calldata _signature,
420:         address _trader
421:     )
422:         external
423:     {
...
433:         (uint _assetPrice,) = tradingExtension.getVerifiedPrice(_trade.asset, _priceData, _signature, 0);

Tools Used

Manual Review

Recommended Mitigation Steps

Use the latestRoundData function instead, and implement sufficient checks to ensure the price returned is valid, by checking roundId and answeredInRound.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #655

[c4-judge]

GalloDaSballo marked the issue as satisfactory