eta - 1. Chainlink aggregators may provide inaccurate prices if they fall below the minAnswer threshold; 2. Potential Data Access Issue in `DelayedOrder: _executeStableWithdraw Function`

[sherlock-admin]

eta

medium

1. Chainlink aggregators may provide inaccurate prices if they fall below the minAnswer threshold; 2. Potential Data Access Issue in DelayedOrder: _executeStableWithdraw Function

1.Chainlink aggregators may provide inaccurate prices if they fall below the minAnswer threshold

Summary

Chainlink aggregators utilize circuit breakers, such as minAnswer and maxAnswer, to prevent extreme price fluctuations from affecting reported prices. However, in scenarios where an asset's value drops below the minAnswer threshold, the aggregator may still return the minAnswer value instead of the actual price. This discrepancy can lead to users borrowing against assets at inflated values, posing significant risks to protocol integrity and potential loss of funds. To mitigate this issue, implementing checks to revert transactions when prices fall outside predetermined bounds is recommended.

Vulnerability Detail

Chainlink aggregators may return incorrect prices if the asset's value falls below a predetermined threshold, known as minAnswer. This feature acts as a circuit breaker to prevent extreme price fluctuations from affecting the reported price. However, in cases of significant asset devaluation, such as the LUNA crash, the aggregator may continuously return the minAnswer instead of the actual price, leading to inaccuracies in value assessment.

Chainlink's latestRoundData function retrieves round data from the associated aggregator, which includes parameters like minAnswer and maxAnswer. If the asset's price falls below minAnswer, the protocol will still value it at minAnswer, potentially enabling users to exploit the system. This discrepancy poses a significant risk to the protocol's integrity and could result in financial losses, as observed in the Venus protocol's experience with LUNA on BSC.

To address this issue, it is advisable to implement a check in the protocol to revert transactions if the price received from the oracle falls outside of predetermined bounds. For example, When TokenA, with a minimum price set at $1, experiences a drop in value to $0.010, the aggregator continues to report a price of $1. Consequently, users are able to borrow against TokenA under the assumption that its value remains at $1, which is 100x its actual value.The protocol should detect this deviation and prevent users from borrowing against TokenA based on an inflated value. This measure would help maintain the protocol's stability and protect users from potential exploitation as shown below.

Impact

If an asset experiences a crash, like LUNA, the protocol can be exploited to provide loans at inflated prices.

Code Snippet

OracleModule::_getOnchainPrice

Tool used

Manual Review

Recommendation

The ChainlinkAdapterOracle needs to verify that the returned answer aligns with the minPrice/maxPrice parameters and should revert if the answer falls outside of these boundaries.

(, int256 _price, , uint256 updatedAt, ) = oracle.latestRoundData();
+   if (price >= maxPrice or price <= minPrice) revert();

2.Potential Data Access Issue in DelayedOrder: _executeStableWithdraw Function

Summary

In the provided code snippet, the function _executeStableWithdraw handles the execution of stable withdrawals for user accounts. However, there's a potential issue where the function _prepareExecutionOrder is called before retrieving necessary data for the withdrawal. This could result in the data being deleted from the storage mapping before it's accessed, leading to errors during the withdrawal process. To resolve this, it's crucial to ensure that the data retrieval occurs before any functions that may modify or delete it.

Vulnerability Detail

In the DelayedOrder contract, there's a function _executeStableWithdraw that involves executing a stable withdrawal for a user account. One critical step within this function is the _prepareExecutionOrder function, which prepares the execution order before executing the stable withdrawal.

However, there seems to be a potential issue with the order in which these functions are executed. If _prepareExecutionOrder is called before retrieving the order.orderData needed for the stable withdrawal, it may result in the orderData being deleted from the _announcedOrder mapping before it can be accessed. This could lead to an error where the stable withdrawal function is unable to retrieve the necessary data for execution, potentially causing unexpected behavior or failure in the withdrawal process.

To address this issue, it's essential to ensure that the _prepareExecutionOrder function is called after retrieving the order.orderData to guarantee that the required data is available during the execution of the stable withdrawal. This ensures that the stable withdrawal process can proceed smoothly without encountering any errors related to missing data.

Impact

If the function _prepareExecutionOrder is executed before retrieving the necessary data for the stable withdrawal in the _executeStableWithdraw function, it could lead to the deletion of crucial data from the storage mapping before it is accessed.

Code Snippet

DelayedOrder::_executeStableWithdraw

Tool used

Manual Review

Recommendation

It's essential to ensure that the _prepareExecutionOrder function is called after retrieving the order.orderData to guarantee that the required data is available during the execution of the stable withdrawal.

function _executeStableWithdraw(address account) internal returns (uint256 amountOut) {
...
-        _prepareExecutionOrder(account, order.executableAtTime);

        FlatcoinStructs.AnnouncedStableWithdraw memory stableWithdraw = abi.decode(
            order.orderData,
            (FlatcoinStructs.AnnouncedStableWithdraw)
        );
+        _prepareExecutionOrder(account, order.executableAtTime);

...

Duplicate of #134

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid