search

code-423n4 / 2023-07-moonwell-findings

1 stars 0 forks source link

Current `setUnderlyingPrice` and `setDirectPrice` open to incorrect liquidation of users' positions and result in financial losses for users #342

Closed code423n4 closed 11 months ago

code423n4 commented 12 months ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/Oracles/ChainlinkOracle.sol#L118-L138

Vulnerability details

Impact

Price feeds can be affected by network congestion, causing transactions with outdated prices to be treated as current prices. As price feeds are crucial to the protocol's functioning, this situation can lead to incorrect liquidation of users' positions and result in financial losses for users.

Proof of Concept

In ChainlinkOracle.sol, the getPrice contains two way of sourcing the price, via Chainlink and via manual input by admin.

As we can check from getPrice function, Line 79-83, if prices[address(token)] != 0, it will get the price from manual input by admin, otherwise, get from chainlink.

File: ChainlinkOracle.sol
69:     /// @notice Get the underlying price of a token
70:     /// @param mToken The mToken to get the underlying price of
71:     /// @return price The underlying asset price mantissa scaled by 1e18
72:     /// @dev if the admin sets the price override, this function will
73:     /// return that instead of the chainlink price
74:     function getPrice(MToken mToken) internal view returns (uint256 price) {
75:         EIP20Interface token = EIP20Interface(
76:             MErc20(address(mToken)).underlying()
77:         );
78:
79:         if (prices[address(token)] != 0) {
80:             price = prices[address(token)];
81:         } else {
82:             price = getChainlinkPrice(getFeed(token.symbol()));
83:         }
84:
85:         uint256 decimalDelta = uint256(18).sub(uint256(token.decimals()));
86:         // Ensure that we don't multiply the result by 0
87:         if (decimalDelta > 0) {
88:             return price.mul(10 ** decimalDelta);
89:         } else {
90:             return price;
91:         }
92:     }

If we check again, where the prices[asset] manually inputed is via two function,

  1. setUnderlyingPrice(MToken mToken, uint256 underlyingPriceMantissa)
  2. setDirectPrice(address asset, uint256 price)
File: ChainlinkOracle.sol
115:     /// @notice Set the price of an asset overriding the value returned from Chainlink
116:     /// @param mToken The mToken to set the price of
117:     /// @param underlyingPriceMantissa The price scaled by mantissa of the asset
118:     function setUnderlyingPrice(
119:         MToken mToken,
120:         uint256 underlyingPriceMantissa
121:     ) external onlyAdmin {
122:         address asset = address(MErc20(address(mToken)).underlying());
123:         emit PricePosted(
124:             asset,
125:             prices[asset],
126:             underlyingPriceMantissa,
127:             underlyingPriceMantissa
128:         );
129:         prices[asset] = underlyingPriceMantissa;
130:     }
131:
132:     /// @notice Set the price of an asset overriding the value returned from Chainlink
133:     /// @param asset The asset to set the price of
134:     /// @param price The price scaled by 1e18 of the asset
135:     function setDirectPrice(address asset, uint256 price) external onlyAdmin {
136:         emit PricePosted(asset, prices[asset], price, price);
137:         prices[asset] = price;
138:     }

Both functions require only two parameters: the token and the price. The timestamp of the price is not considered in these functions. As a consequence, the price feeds may encounter disruptions during network congestion, leading to the acceptance of stale prices as if they were up-to-date.

Since the accuracy of price feeds is critical to the protocol's operations, this situation could potentially result in users' positions being liquidated incorrectly and, in turn, cause financial losses for users.

Proof of Concept:

  1. Alice borrowed 50,000 USDC with 1 ETH as collateral.
  2. Ethereum price dropped to $90,000. To avoid liquidation, Alice repaid 10,000 USD.
  3. The price of Ethereum dropped to $80,000. The administrator of ChainlinkOracle.sol attempted to setDirectPrice() with the latest price: $80,000. However, due to network congestion, the transaction was not mined timely.
  4. Ethereum price rebounded to $100,000. Alice borrowed another 10,000 USDC.
  5. The transaction sent by the administrator in step 3 was finally mined. The protocol now believes the price of Ethereum has suddenly dropped to $80,000, resulting in the liquidation of Alice's position.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider to add timestamp and time threshold to prevent any stale price

Assessed type

Oracle

0xSorryNotSorry commented 12 months ago

The admin's overriding price set is not due to stale prices but is an intervention of a situation like depeg events etc. So one should not expect a continous dynamis price feed of the referred assets. The implementation is in the intended behaviour.

Invalid assumption.

c4-pre-sort commented 12 months ago

0xSorryNotSorry marked the issue as low quality report

c4-judge commented 11 months ago

alcueca marked the issue as unsatisfactory: Invalid