Collateral value can be manipulated due to missing circuit breaker checks

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L336-L340

Vulnerability details

Impact

V3Oracle::getValue() will return an incorrect amount, over-inflating the value of the attacker’s collateral. The attacker can utilise this to borrow significantly more than they should.

Proof of Concept

A Chainlink Oracle is used to fetch the price of a pool in V3Oracle::getChainlinkPriceX96(), V3Oracle.sol#L337-L340:

(, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();
if (updatedAt + feedConfig.maxFeedAge < block.timestamp || answer < 0) {
    revert ChainlinkPriceError();
}

There is no check to ensure the answer returned by latestRoundData() is not above or below a certain price. Some Chainlink aggregators have a built in circuit breaker. This is a predetermined price band set in a pool’s AccessControlledOffchainAggregator contract, OffchainAggregator.sol#L56-L59:

// Lowest answer the system is allowed to report in response to transmissions
int192 immutable public minAnswer;
// Highest answer the system is allowed to report in response to transmissions
int192 immutable public maxAnswer;

If the price of a pool is less than minAnswer or greater than maxAnswer then answer returned by the oracle will still be minAnswer or maxAnswer respectively. For example: the price of a pool could be 100 yet if minAnswer is 1e13 then latestRoundData() will return answer as 1e13. The protocol attempts to mitigate against this issue by utilising a dual-oracle system. Whereby the Chainlink price is checked against the Uniswap pool price and if the difference is greater than maxPoolPriceDifference the transaction will revert. However there is a issue. In V3Oracle::_checkPoolPrice() the Uniswap pool price is retrieved by calling _getReferencePoolPriceX96() — where 0 is passed as twapSeconds, V3Oracle.sol#L135:

uint256 priceX96 = _getReferencePoolPriceX96(pool, 0);

If twapSeconds is 0 then the sqrtPriceX96 is assigned to pool.slot0(). This is the price based on the current state of the pool, V3Oracle.sol#L360-L364:

uint160 sqrtPriceX96;
// if twap seconds set to 0 just use pool price
if (twapSeconds == 0) {
    (sqrtPriceX96,,,,,,) = pool.slot0();
} else {

The problem is that retrieving the price as such is extremely risky as Uniswap pool prices can be easily manipulated using flash loans. An attacker can perform the following exploit due to this:

1. The price of a pool drops below minAnswer to 100 — latestRoundData() returns answer as minAnswer. For this example minAnswer will be 1e13.
2. The attacker calls V3Vault::borrow() passing some large amount of assets. Simultaneously they use a flash loan to manipulate the Uniswap pool price, bringing the price back to 1e13.
3. Since the Chainlink price and the Uniswap price are within the maxPoolPriceDifference, the attacker will have over-inflated their collateral value and borrowed a large amount of funds.

Note: this finding builds on an automated finding — the impact of this finding is higher due to a novel attack vector not mentioned in the automated finding.

Tools Used

Manual review

Recommended Mitigation Steps

The protocol should implement its own circuit breaker min and max values for each pool and validate them in _getChainlinkPriceX96(), V3Oracle.sol#L337:

        (, int256 answer,, uint256 updatedAt,) = feedConfig.feed.latestRoundData();
        ...
+       require(answer >= MIN_PRICE && answer <= MAX_PRICE, "Invalid Price");

Assessed type

Oracle

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-pre-sort]

0xEVom marked the issue as insufficient quality report

[0xEVom]

This is a great description of how #13 could be exploited, but I don't think the higher severity is warranted seeing as per the Chainlink docs:

On most data feeds, [minAnswer and maxAnswer] are no longer used.

OOS #13

[jhsagd76]

maxAnswer and minAnswer have been abandoned. Currently, no Chainlink oracle will return when below the minAnswer; they will only expire. If not, please give me an example.

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Out of scope

[0x175]

Hi @jhsagd76,

Offering examples is something I can certainly do. Here are five popular price feeds and their corresponding minAnswer values:

1. USDC/ETH: 0x986b5E1e1755e3C2440e960477f25201B0a8bbD4 (1e13),
2. USDT/ETH: 0xEe9F2375b4bdF6387aa8265dD4FB8F16512A1d46 (1e13),
3. MKR/ETH: 0x24551a8Fb2A7211A25a17B1481f043A8a8adC7f2 (1e15),
4. UNI/ETH: 0xD6aA3D25116d8dA79Ea0246c4826EB951872e02e (1e14),
5. LINK/ETH: 0xDC530D9457755926550b59e8ECcdaE7624181557 (1e14)

It is partially true that as you mentioned maxAnswer and minAnswer have been abandoned, however this is only for most price feeds as suggested here:

The data feed aggregator includes both minAnswer and maxAnswer values. On most data feeds, these values are no longer used and they do not stop your application from reading the most recent answer.

There is still a substantial amount of price feeds that have a minAnswer value that is susceptible to this attack, as I indicated above. The more price feeds that the protocol utilises, the higher the likelihood of this attack occurring if not properly mitigated. Chainlink is largely a centralised system and when relying on their data it is important to avoid making assumptions when retrieving data for a price feed — especially when user’s funds are at risk and it is made explicitly clear that only most data feeds return a reliable answer, not all. Chainlink recommends:

For monitoring purposes, you must decide what limits are acceptable for your application. Configure your application to detect when the reported answer is close to reaching reasonable minimum and maximum limits so it can alert you to potential market events. Separately, configure your application to detect and respond to extreme price volatility or prices that are outside of your acceptable limits.

Which is also precisely what I recommended to mitigate this attack. As mentioned in the submission, this finding builds on #13. However, the root cause can be attributed to two factors: 1) not implementing custom circuit breaker checks as recommended by Chainlink and myself, and 2) the use of slot(0) to retrieve the price from a Uniswap pool, which is susceptible to manipulation — this second factor is currently being judged as a valid finding, #175.

[jhsagd76]

Let me further explain why minAnswer has been deprecated. First, the default configuration for Chainlink has now set this value to 1, so new quotes will no longer be affected by it. Second, for older aggregators, issues like the LUNA circuit breaker only occur in highly volatile markets because the minAnswer circuit breaker check will prevent off-chain nodes from forming a price consensus that passes the circuit breaker check, thus the quoting will be interrupted directly instead of using the minAnswer quote, thats I said "they will only expire. If not, please". Lastly, even in markets with extreme volatility, due to the protocol's TWAP check, this issue will not occur.

I've seen similar statements In many reports of other contests, "If the price of a pool is less than minAnswer or greater than maxAnswer then answer returned by the oracle will still be minAnswer or maxAnswer respectively.," which are incorrect. The current implementation of Chainlink DON does not support this argument.

BTW, link this issue to the 175 makes sense. TWAP check can be skiped. Its a new attack case but with the same root cause of slot0 price.

[c4-judge]

jhsagd76 marked the issue as duplicate of #175

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 changed the severity to 2 (Med Risk)