Medium - price difference is calculated using the on-chain price only

Summary

When calculating the difference between the price reported by Chainlink and the price reported by the Pyth oracle inside of OracleModule.sol, only the price from Chainlink is used, which can lead to situations where the difference in price is outside the acceptable threshold if the sources of the prices were switched, (If CL price was from Pyth and vice versa), if only the off-chain price was considered, or even if both prices were taken into account.

Vulnerability Detail

This getPrice() function is used throughout the entire protocol in multiple modules, which means this could have many undesired effects. Especially considering that directly after the price difference, diffPercent, is calculated, the price returned to the protocol is either the price returned by Pyth or the price returned by Chainlink. This is due to the fact that the protocol uses the "freshest price". As long as the on-chain price is larger than the off-chain price, the diffPercent variable will always be smaller than it would be if calculated with the off-chain price.

Impact

With everything we have discussed already in mind, apply this to any leveraged positions and liquidations since they are one group that would be directly affected by this and could lose their funds. For this example we will say that the protocol's maxDiffPercent is 10%, the current price of rETH is $1000, and Alice opens a leveraged position with a liquidation threshold at $900 rETH.

When checking if Alice's position is liquidatable, we will use OracleModule's getPrice() function. If Chainlink returns $1000 as the price, but the Pyth oracle returns $900, the current priceDiff would be $100 (uint256 priceDiff = (int256(onchainPrice) - int256(offchainPrice)).abs();). We get a diffPercent of 10% using the current logic (uint256 diffPercent = (priceDiff * 1e18) / onchainPrice;). This doesn't trigger the PriceMismatch custom error if (diffPercent > maxDiffPercent) revert FlatcoinErrors.PriceMismatch(diffPercent); and now the price could be returned as either of the returned values, potentially liquidating Alice despite the price difference being 11.1% when calculating using the off-chain price. To put this into perspective further, if the on-chain price was $900 and the off-chain price was $1000, literally the same values as in our example except for the sources are swapped, the PriceMismatch custom error would've been thrown.

Code Snippet

This function is in the OracleModule.sol file, you can view it here.

function _getPrice(uint32 maxAge) internal view returns (uint256 price, uint256 timestamp) {
        (uint256 onchainPrice, uint256 onchainTime) = _getOnchainPrice(); // will revert if invalid
        (uint256 offchainPrice, uint256 offchainTime, bool offchainInvalid) = _getOffchainPrice();
        bool offchain;
        uint256 priceDiff = (int256(onchainPrice) - int256(offchainPrice)).abs();
        uint256 diffPercent = (priceDiff * 1e18) / onchainPrice; 
        if (diffPercent > maxDiffPercent) revert FlatcoinErrors.PriceMismatch(diffPercent);
        if (offchainInvalid == false) {
            // return the freshest price
            if (offchainTime >= onchainTime) {
                price = offchainPrice;
                timestamp = offchainTime;
                offchain = true;
            } else {
                price = onchainPrice;
                timestamp = onchainTime;
            }
        } else {
            price = onchainPrice;
            timestamp = onchainTime;
        }
        // Check that the timestamp is within the required age
        if (maxAge < type(uint32).max && timestamp + maxAge < block.timestamp) {
            revert FlatcoinErrors.PriceStale(
                offchain ? FlatcoinErrors.PriceSource.OffChain : FlatcoinErrors.PriceSource.OnChain
            );
        }
    }

Tool used

Manual Review

Recommendation

To ensure that the PriceMismatch error is thrown if the difference in the two prices is outside the acceptable deviation, you can alter the getPrice() function in a couple ways:

This method calculates the diffPercent using the smaller of the two prices, which means that the result of the two possible calculations, off-chain vs on-chain, will always be the larger diffPercent.

uint256 diffPercent = (priceDiff * 1e18) / (onChainPrice < offChainPrice ? onChainPrice : offChainPrice);

Returns 11.11% with our example

And this method calculates the diffPercent for both prices, and then gets the average between them:

uint256 onchainDiff= (priceDiff * 1e18) / onChainPrice; 
uint256 offchainDiff = (priceDiff * 1e18) / offChainPrice; 
uint256 diffPercent = (onchainDiff + offchainDiff) / 2;

Returns 10.55% with our example

If either of these solutions were implemented, then our example function call would always revert with PriceMismatch.

Duplicate of #177

sherlock-admin commented 8 months ago

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

takarez commented:

valid: the diffPercent should work the same even when the values are swapped; medium(1)

sherlock-admin commented 7 months ago

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/270.

sherlock-audit / 2023-12-flatmoney-judging

trauki - Medium - price difference is calculated using the on-chain price only #87