The logic behind MIN_PRICE_DIFFERENCE is heavily flawed as it allows for heavy arbitraging

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L190-L196

Vulnerability details

Proof of Concept

The V3Oracle contract defines maxPoolPriceDifference to regulate the maximum price difference allowed between the oracle price and the pool price. This parameter is crucial for maintaining the integrity and reliability of price information used in financial decisions.

Keep in mind that the _maxPoolPriceDifference cannot be set to be lower than the already stored MIN_PRICE_DIFFERENCE
https://github.com/code-423n4/2024-03-revert-lend/blob/457230945a49878eefdc1001796b10638c1e7584/src/V3Oracle.sol#L190-L196

    function setMaxPoolPriceDifference(uint16 _maxPoolPriceDifference) external onlyOwner {
        if (_maxPoolPriceDifference < MIN_PRICE_DIFFERENCE) {
            revert InvalidConfig();
        }
        maxPoolPriceDifference = _maxPoolPriceDifference;
        emit SetMaxPoolPriceDifference(_maxPoolPriceDifference);
    }

Now initially we can see that maxPoolPriceDifference is set equal to MIN_PRICE_DIFFERENCE which is 200, i.e 2%

uint16 public maxPoolPriceDifference = MIN_PRICE_DIFFERENCE; // max price difference between oracle derived price and pool price x10000

Now given that the maximum price difference is set at a value that translates to a 2% tolerance, this threshold may not be stringent enough for protocol and actually causes arbitrageurs to come and heavily game the system.
This setting implies that the system could accept and act upon price data that is up to 2% away from the real market price, i.e Ethereum is set to be reaching $5000 in the coming days, this means that protocol is allowing arbitrageurs to go away with $100 as a spread, i.e say the current mode is CHAINLINK_TWAP_VERIFY the twap could return a value 1.9% higher than the real life value and protocol would still integrate with this couple that with the fact that protocol implements leveraging logic and that uniswap oracles which are unreliable on L2s are used this would lead to a leak of value

Impact

The V3Oracle contract specifies a maximum allowable price difference between the oracle-derived price and the actual pool price, with a set tolerance of 2% (maxPoolPriceDifference = MIN_PRICE_DIFFERENCE). This threshold is intended to mitigate the risks associated with minor price discrepancies due to market volatility or latency in oracle updates. However, setting this tolerance to 2% is excessive. Such a wide margin for price difference would not only lead to the acceptance of faulty price data, but also a leak of value to the protocol by arbitragers as they can carefully chose their trading decisions to game the system.

Recommended Mitigation Steps

Reconsider the value of MIN_PRICE_DIFFERENCE or atleast introduce a functionality to be able to change the value after deployment of protocol.

Assessed type

Context

[c4-pre-sort]

0xEVom marked the issue as insufficient quality report

[0xEVom]

Insufficient proof

[jhsagd76]

I think this is a valuable suggestion, but if I retain the current grade of this issue, we will fall into endless debates. For now, this issue will be marked as L-A. If the warden provides a PoC during the post-QA period that proves a 2% difference is sufficient to extract profits greater than 1 dollar by directly manipulate the uniswap pool(especially stablecoin pool), I will upgrade this issue to M.

It can't be H because of the huge cost of manipulating twap.

[c4-judge]

jhsagd76 changed the severity to QA (Quality Assurance)

[c4-judge]

jhsagd76 marked the issue as grade-a

[jhsagd76]

Looking forward to the sponsor's view on this issue.

[kalinbas]

We would agree to set the lowest possible value to 1%. Also with the change in #175 this check becomes less relevant

[c4-sponsor]

kalinbas (sponsor) confirmed

[kalinbas]

After discussion we think it might be better to remove MIN_PRICE_DIFFERENCE and leave it completely configurable. Changes in Oracle will be made with a Timelock contract anyways, and the risk is that protocol owner could configure a very small value and break borrowing and liquidations (which he could do as well just by setting an invalid TokenConfig)

[Bauchibred]

Hi @jhsagd76, thanks for judging, in regards to this

.

I think this is a valuable suggestion, but if I retain the current grade of this issue, we will fall into endless debates. For now, this issue will be marked as L-A. If the warden provides a PoC during the post-QA period that proves a 2% difference is sufficient to extract profits greater than 1 dollar by directly manipulate the uniswap pool(especially stablecoin pool), I will upgrade this issue to M.

I believe this bug case has been a bit misunderstood, the bug is not only about “directly manipulating a uniswap pool” but rather heavily built on the fact that the difference in price provided by the oracles is accepted to be +/- 2% different and that’s the minimum it could be, it can only be set higher.

Just like the short case attached in the report, say ETH is priced at $5000, protocol is accepting a potential minimum spread of $100, and if BTC is the case we are looking at a whopping $1400 (assuming a price of $70,000), considering there is still a leveraging logic pertaining to protocol this is risky.

Key to note that even popularly well known oracle providers have their price difference updaters set at 0.5% range so as not to allow arbitrageurs game systems using their provided price, for example using Chainlink as a reference and the two tokens whose ideas have been passed in this report/escalation, we can see that for both the ETH and BTC feed, if the price difference in comparison to the real value of the token is more than 0.5% it updates, see sources, 1 and 2, where they both have a deviation of 0.5% or heartbeat of 3600 so new price triggers are passed, whenever the price goes 0.5% above/below real value or after an hour.

The above showcases that the logic on accepting the price at a 2% higher rate or 2% lower rate is generally flawed, considering as we all know that where as manipulating say the TWAP on the mainnet is costly and somewhat hard to come by, it’s not “impossible” and also, protocol is going to be deployed on other EVM compatible chains, where the TWAP oracles on these chains are easier to manipulate.

Also this bug case is similar to other previously accepted bug cases, for example take a look at this H-01 finding in an audit on Code4rena from Q4 2023: https://code4rena.com/reports/2023-11-kelp#h-01-possible-arbitrage-from-chainlink-price-discrepancy- the logic is somewhat similar as even the price discrepancy being discussed is the 2% being accepted (whereas the logic here is a tad different, it’s still been attached to make the PJQA comment whole).

Finally we can see instances from this audit contest where the sponsor’s decision have decided if a finding should be awarded as a H/M or not, an example can be seen in here so we would like the same idea to be applied here since the sponsors have confirmed this finding and initially wanted to change the MIN_PRICE_DIFFERENCE to a low value of 1% but concluded on the decision that they are going to apply the suggested fix from the report of allowing the min price difference to be changed after deployment or in this case sponsors plan on scrapping the whole logic and allow it to be configurable, which also helps mitigate this as more stringent percentages can be placed for pairs that are not too volatile and otherwise for pairs that are volatile.

With the above, we believe we’ve stated fact-based comments and provided references on how this report should be a valid H/M finding, so we’d appreciate your reassessment, thanks.

[jhsagd76]

This is a completely different situation from the report you mentioned, and it's also not the same as Chainlink's deviation. PRICE_DIFFERENCE is only used to check the difference between pool price and backup price, and is only used for the calculation of the health factor. Therefore, loosening this restriction does not lead to a direct security risk. Moreover, lowering this configuration to below 1% is dangerous as it would significantly impact normal functioning and liquidations. This is an inherent issue with lending DeFis; for example, I was involved in the development of the largest lending protocol on Solana, which faced continuous failures of several minutes for our liquidation bots due to strict confidence intervals and TWAP restrictions, and we had to adopt different ranges of tolerances for different behaviors. I am quite certain that reducing this value to 0.5% would lead to more severe risks.

In summary, this is completely unrelated to the issue you linked to.

I am confident that the sponsors fully known the additional implications of this configuration. @kalinbas

[Bauchibred]

Hi @jhsagd76, thanks for taking the time to go through the comments and the report once again, we’re convinced on how the referenced cases have different contexts and are not disagreeing on that fact, apologies for the time you spent checking them.

Back to this issue in question, and your comment here

This is an inherent issue with lending DeFis; for example, I was involved in the development of the largest lending protocol on Solana, which faced continuous failures of several minutes for our liquidation bots due to strict confidence intervals and TWAP restrictions, and we had to adopt different ranges of tolerances for different behaviors.

Considering the way this was fixed in that instance, I.e “we had to adopt different ranges of tolerances for different behaviors.” This point was also both stated in the report and escalation, albeit we only talked about reduction in the report i.e our recommendation included “introducing a functionality to be able to change the value of ‘MIN_PRICE_DIFFERENCE’ after deployment of protocol.” in the escalation we stated setting it higher/lower depending on the current behaviour.

I am confident that the sponsors fully known the additional implications of this configuration. @kalinbas

Not to jump in before the sponsors, but quoting them:

We would agree to set the lowest possible value to 1%. Also with the change in #175 this check becomes less relevant

After discussion we think it might be better to remove MIN_PRICE_DIFFERENCE and leave it completely configurable. Changes in Oracle will be made with a Timelock contract anyways, and the risk is that protocol owner could configure a very small value and break borrowing and liquidations (which he could do as well just by setting an invalid TokenConfig)

Considering the latter is going to be the way they approach this and they set it dependent on the behaviour, then this would now be okay with it being highly configurable, no? Since it’s no longer a constant and they can pass a change via the timelock whenever the need arises.

[mariorz]

While we are indeed removing the MIN_PRICE_DIFFERENCE and wanted to state that in this issue that also pertains to the MIN_PRICE_DIFFERENCE, it is not in any way a confirmation of the validity of this issue.

The issue claims that a 2% setting would lead to economic attacks, yet does not show how that would be executed. We agree with the QA classification.

[kalinbas]

revert-finance/lend#9