Tigris Trade contest - 0xDecorativePineapple's results

Lines of code

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

Vulnerability details

Impact

It has been identified that the Tigris project incorrectly handles the decimals of ERC20 tokens. Specifically:

1. In the StableVault.sol contract, the calculation that scales the value of the deposited amount assumes that the _token supplied by the user has less than 18 decimals: _amount*(10**(18-IERC20Mintable(_token).decimals())). This is not always true, as ERC20 tokens may have more than 18 decimals, resulting in a contract call that will revert.

2. There are some places in the codebase that ERC20 tokens are assumed to have 18 decimals. For example, in the TradingLibrary.sol:verifyPrice() function (https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/TradingLibrary.sol#L115), the price provided in the PriceData struct is assumed to have 18 decimals. The contract then scales the price retrieved from a Chainlink feed to 18 decimals, assuming that the provided price(in the PriceData struct ) has 18 decimals as well. This verifyPrice function is used to verify that the price provided in the PriceData struct is close to the one fetched from Chainlink. However, if the decimals of the ERC20 token are different than 18, this could result in incorrect calculations and even prevent trades from being initiated/closed or margin from being removed.

Tools Used

• Manual code review

Recommended Mitigation Steps

To handle the fact that ERC20 token decimals may be different than 18, a solution can be implemented to scale the token decimals between different ERC20 tokens. An example of this can be found in Chainlink's scalePrice function (https://docs.chain.link/docs/get-the-latest-price/#getting-a-different-price-denomination).

Lines of code

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

Vulnerability details

Impact

The latestAnswer() function for ERC20 is deprecated according to Chainlink's documentation. This can lead to the following issues:

• If _chainlinkEnabled is set and Chainlink no longer supports deprecated APIs, the contract may face a Denial of Service (DoS), as prices cannot be obtained and new trades cannot be initiated or executed.
• The returned price may be stale. The latestAnswer function only returns the value of the latest price, whereas latestRoundData returns additional information that can be used to validate whether a price is stale.

Proof of Concept

The verifyPrice function uses the deprecated latestAnswer() API:

function verifyPrice(
        ...
        if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
            int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
            if (assetChainlinkPriceInt != 0) {
             ..
}

Tools Used

Manual Code Review

Recommended Mitigation Steps

It's recommended to use the latest latestRoundData function and validate the oracle response for freshness and round completion in addition to nonzero price:

 (uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = IPrice(_chainlinkFeed).latestRoundData();
    require(answeredInRound >= roundID, "...");
    require(timeStamp != 0, "...");
    require(price != 0, "...");

For more information, please refer to the Chainlink documentation on latestAnswer.