branch_indigo - Collaterals might be incorrectly valued when DSU or USDC depegs leading to under collateralized positions

[sherlock-admin]

branch_indigo

medium

Collaterals might be incorrectly valued when DSU or USDC depegs leading to under collateralized positions

Summary

When DSU or USDC depegs, user collaterals will be incorrectly valued, potentially causing user taking on leverages unsafe to the products and protocol.

Vulnerability Detail

Both a wrapped coin and a stable coin can depeg due to many factors. This has happened more than once. The recent incident is USDC, Dai, and USDT depeg due to sillcon valley bank collapse. A depeg tends to lead to immediate panic trading and increasing arbitrage that destabilizes defi markets.

When a user deposits, they either deposit USDC at the front end or DSU directly to the smart contract. Eventually, DSU is stored as collaterals. When an account is evaluated for liquidation, the positions are multiplied by oracle prices than scaled based on a maintenance factor, the number is then compared directly with the collateral DSU amount.

//Collateral.sol
    function liquidatable(address account, IProduct product) external view returns (bool) {
        if (product.isLiquidating(account)) return false;
|>        return product.maintenance(account).gt(collateral(account, product));
    }

//AccountPosition.sol
    function _maintenance(Position memory position) private view returns (UFixed18) {
        IProduct product = IProduct(address(this));
        Fixed18 oraclePrice = product.currentVersion().price;
|>       UFixed18 notionalMax = Fixed18Lib.from(position.max()).mul(oraclePrice).abs();
        return notionalMax.mul(product.maintenance());
    }

//Collateral.sol
    function collateral(address account, IProduct product) public view returns (UFixed18) {
|>        return _products[product].balances[account];
    }

Since in ChainlinkOracle.sol, prices are supposedly fetched denominated in USD, this comparison equates USD with DSU. While this is fine in most cases, when USDC or DSU depegs, this equation would be broken and there is no mechanism in current implementation to check or handle this.

//ChainlinkOracle.sol
    function currentVersion() public view returns (OracleVersion memory oracleVersion) {
        return _buildOracleVersion(registry.getLatestRound(base, quote));
    }

//ChainlinkRegistry.sol
    function getLatestRound(ChainlinkRegistry self, address base, address quote) internal view returns (ChainlinkRound memory) {
        (uint80 roundId, int256 answer, , uint256 updatedAt, ) =
            FeedRegistryInterface(ChainlinkRegistry.unwrap(self)).latestRoundData(base, quote);
        return ChainlinkRound({roundId: roundId, timestamp: updatedAt, answer: answer});
    }

//ChainlinkOracle.sol
    function _buildOracleVersion(ChainlinkRound memory round, uint256 version)
    private view returns (OracleVersion memory) {
        Fixed18 price = Fixed18Lib.ratio(round.answer, _decimalOffset);
        return OracleVersion({ version: version, timestamp: round.timestamp, price: price });
    }

Impact

When DSU drops significantly below us dollar, users can be under collateralized putting protocol and product at risks. When the depegs happen, it could take place swiftly with significant price drops. Without an on-chain check, a protocol-wide pause might be needed after the fact to halt all activities, however, such a remedy will not be prompt to prevent early and drastic impact on users and products.

Code Snippet

https://github.com/equilibria-xyz/perennial-mono/blob/b06d5145db62a312dd88dfcafef0f8e2166c5e32/packages/perennial-oracle/contracts/ChainlinkOracle.sol

https://github.com/equilibria-xyz/perennial-mono/blob/b06d5145db62a312dd88dfcafef0f8e2166c5e32/packages/perennial-oracle/contracts/types/ChainlinkRegistry.sol#L104-L107

https://github.com/equilibria-xyz/perennial-mono/blob/b06d5145db62a312dd88dfcafef0f8e2166c5e32/packages/perennial-oracle/contracts/ChainlinkOracle.sol

Tool used

Manual Review

Recommendation

Consider adding check for DSU price deviation in ChainlinkOracle.sol. For example, in _buildOracleVersion(ChainlinkRound memory round, uint256 version), add a check for DSU price in a separate price feed like UniswapV3, and round timestamp can be used to access a record price when needed.

[bzpassersby]

Escalate for 10 USDC I think this should be valid. DSU is pegged to USDC, not USD. Even though the protocol accepts a strict 1:1 peg between DSU and USDC, USDC can still depeg from USD. And USDC depeg just happened recently during silicon valley bank collapse. This is a real risk. And because maintenance is directly compared with collateral DSU amount. It is necessary to add check for DSU price deviation.

[sherlock-admin]

Escalate for 10 USDC I think this should be valid. DSU is pegged to USDC, not USD. Even though the protocol accepts a strict 1:1 peg between DSU and USDC, USDC can still depeg from USD. And USDC depeg just happened recently during silicon valley bank collapse. This is a real risk. And because maintenance is directly compared with collateral DSU amount. It is necessary to add check for DSU price deviation.

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.

[KenzoAgada]

My reasoning here is similar to #17 : I think this is a kind of a general issue that is stating the obvious, a design choice, and the sponsor is aware of it, so there is no much value in such a submission. (However I did admit in #17 that there may be value in raising such an issue in an audit). In #17 I asked the sponsor for his perspective and he agreed with me.

Therefore in this issue I maintain the same position, that this is more of a general design of the protocol, and not a valid issue in Sherlock. Sherlock judge can choose whether to change the validity and/or alert the sponsor to this issue.

[jacksanford1]

As far as I can tell, the Perennial protocol does not care what the price of 1 USDC is in dollar terms. It is the user's responsibility to monitor the dollar value of USDC.

But maybe @arjun-io @kbrizzle can confirm that their Chainlink call will not involve a USD market?

[arjun-io]

Currently some products do use USD denominated price feeds. However, it is correct that aside from ease of user calculations, there is nothing in the protocol that requires USDC maintaining the peg. All accounting remains correct but effectively the exposure is dampened by a depeg. Because Perennial products are closed sum (every 1 USDC of profit must be accounted for by 1 USDC of loss, but that 1 USDC may not result in 1 USD when converting) the feeds are not used to price collateral.

The reasoning posted here is similar: https://github.com/sherlock-audit/2023-05-perennial-judging/issues/180#issuecomment-1601570323

[jacksanford1]

Thanks Arjun.

@bzpassersby Any response here? Any way to refute this statement?

there is nothing in the protocol that requires USDC maintaining the peg

[jacksanford1]

Result: Invalid Unique Invalid based on Arjun's last comment and lack of update from escalator.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• bzpassersby: rejected