search

sherlock-audit / 2023-05-Index-judging

6 stars 3 forks source link

Bauchibred - Inadequate Protection Against Negative or Zero Price #171

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Bauchibred

medium

Inadequate Protection Against Negative or Zero Price

Summary

The _createActionInfo() function does not handle scenarios where the price data fetched from a Chainlink oracle is negative or zero. This lack of protection can result in improper computations and faulty valuations within the contract, leading to potential economic implications.

Vulnerability Detail

Chainlink price feeds, used in the contract for real-time price information, return data as int256. While the contract accepts these prices as int256, it doesn't account for the potential of these prices to be negative or zero. Such values are valid within the scope of int256 but are not appropriate for price calculations.

Take a look at AaveLeverageStrategyExtension.sol#L884-L907

    /**
     * Create the action info struct to be used in internal functions
     *
     * return ActionInfo                Struct containing data used by internal lever and delever functions
     */
    function _createActionInfo() internal view returns(ActionInfo memory) {
        ActionInfo memory rebalanceInfo;

        // Calculate prices from chainlink. Chainlink returns prices with 8 decimal places, but we need 36 - underlyingDecimals decimal places.
        // This is so that when the underlying amount is multiplied by the received price, the collateral valuation is normalized to 36 decimals.
        // To perform this adjustment, we multiply by 10^(36 - 8 - underlyingDecimals)
        int256 rawCollateralPrice = strategy.collateralPriceOracle.latestAnswer();
        rebalanceInfo.collateralPrice = rawCollateralPrice.toUint256().mul(10 ** strategy.collateralDecimalAdjustment);
        int256 rawBorrowPrice = strategy.borrowPriceOracle.latestAnswer();
        rebalanceInfo.borrowPrice = rawBorrowPrice.toUint256().mul(10 ** strategy.borrowDecimalAdjustment);

        rebalanceInfo.collateralBalance = strategy.targetCollateralAToken.balanceOf(address(strategy.setToken));
        rebalanceInfo.borrowBalance = strategy.targetBorrowDebtToken.balanceOf(address(strategy.setToken));
        rebalanceInfo.collateralValue = rebalanceInfo.collateralPrice.preciseMul(rebalanceInfo.collateralBalance);
        rebalanceInfo.borrowValue = rebalanceInfo.borrowPrice.preciseMul(rebalanceInfo.borrowBalance);
        rebalanceInfo.setTotalSupply = strategy.setToken.totalSupply();

        return rebalanceInfo;
    }

The issue arises in the following code snippet:

int256 rawCollateralPrice = strategy.collateralPriceOracle.latestAnswer();
rebalanceInfo.collateralPrice = rawCollateralPrice.mul(10 ** strategy.collateralDecimalAdjustment);
int256 rawBorrowPrice = strategy.borrowPriceOracle.latestAnswer();
rebalanceInfo.borrowPrice = rawBorrowPrice.mul(10 ** strategy.borrowDecimalAdjustment);

Here, price data is fetched from Chainlink using the latestAnswer() function, and the returned values are used directly without verifying if they are positive. The fetched prices are then multiplied by a power of 10 based on a decimal adjustment associated with the strategy, which leads to the calculation of collateral and borrow values. The contract doesn't guard against scenarios where the Chainlink price might be negative or zero.

Impact

If the price returned by Chainlink is negative or zero, this will cause incorrect computation of the collateralPrice and borrowPrice. These inaccuracies will propagate to the valuation of collateral and borrow balances, which may be misrepresented.

Code Snippet

AaveLeverageStrategyExtension.sol#L884-L907

Tool used

Manual review

Recommendation

To rectify this vulnerability, it is recommended to incorporate a check that reverts the execution if the Chainlink price is less than or equal to zero. This will ensure that invalid price data does not result in inaccurate token valuations.

It's crucial to consider the edge cases of price data and to include suitable checks to manage these scenarios. This precaution will prevent potential manipulation or unexpected behavior in the contract.

Bauchibred commented 1 year ago

Escalate for 10USDC

I'd like to indicate that this issue unlike #158 is actually not a duplicate of #296

The query to Chainlink can return a negative price in some cases which is why the returned price is being stored using an int and not a uint this is correctly implemented as at lines 895 and 897 of AaveLeverageStrategyExtension.sol prices are being stored using int, but these values are used directly instead of being checked to see if they are positive.

I actually believe the judge might have missed this, this comment was made by judge on the another issue which I believe should make this submission valid and stand on it's own.

Important references:

sherlock-admin commented 1 year ago

Escalate for 10USDC

I'd like to indicate that this issue unlike #158 is actually not a duplicate of #296

The query to Chainlink can return a negative price in some cases which is why the returned price is being stored using an int and not a uint this is correctly implemented as at lines 895 and 897 of AaveLeverageStrategyExtension.sol prices are being stored using int, but these values are used directly instead of being checked to see if they are positive.

I actually believe the judge might have missed this, this comment was made by judge on the another issue which I believe should make this submission valid and stand on it's own.

Important references:

You've created a valid escalation for 10 USDC!

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 commented 1 year ago

Invalid. The linked issue is about synthetics for real world assets like oil. AAVE only has crypto assets on it

The Chainlink Aggregator contracts will only return a price of 0 if the actual price of the token is 0. The Aggregator contracts do not return 0 price to indicate an error condition. This may have been the behavior of previous Aggregator contracts, but this is no longer the behavior of the most recent contracts.

Bauchibred commented 1 year ago

I stand corrected and agree with the points raised by the lead Watson, i.e since there are only crypto assets on AAVE and not real world assets the chances of this happening is 0, would be interesting to see how AAVE is going to integrate tokenised real world assets in the future, if they are ever going to... I guess we cross that bridge when we get there.

hrishibhat commented 1 year ago

Result: Invalid Unique Considering this a non-issue based on above comments

sherlock-admin commented 1 year ago

Escalations have been resolved successfully!

Escalation status: