code-423n4 / 2024-03-ondo-finance-findings

5 stars 6 forks source link

Lack of Slippage Protection in `ousgInstantManager` Contract #186

Open c4-bot-3 opened 6 months ago

c4-bot-3 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/ousgInstantManager.sol#L230-L241 https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/ousgInstantManager.sol#L254-L276 https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/ousgInstantManager.sol#L335-L351 https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/ousgInstantManager.sol#L362-L386

Vulnerability details

Impact

Users may receive less OUSG, rOUSG, USDC than anticipated if the price of OUSG increases or decreases during transaction confirmation due to the lack of a slippage mechanism.

Proof of Concept

The OUSG price has historically shown volatility with sudden surges Please review the chart and switch to the 'Max' timeframe to observe the historical price volatility. If users call the mint function just before a rapid surge in OUSG price, as observed on March'23 and December'23, they may receive fewer tokens than expected due to the price volatility.

for example: Bob has 1 million USDC and wishes to mint OUSG at the current price of 105 USDC per OUSG.

Bob calls the mint function with 1,000,000e6 USDC, which in turn triggers the _mint function. Inside _mint, the getOUSGPrice() is called to fetch the latest OUSG price. Subsequently, _getMintAmount() is invoked to calculate the amount of OUSG to be minted based on the latest price obtained.

  function _getMintAmount(
    uint256 usdcAmountIn,
    uint256 price
  ) internal view returns (uint256 ousgAmountOut) {
    uint256 amountE36 = _scaleUp(usdcAmountIn) * 1e18;
    ousgAmountOut = amountE36 / price;
  }

function _scaleUp(uint256 amount) internal view returns (uint256) {

// decimalsMultiplier = 1e12
    return amount * decimalsMultiplier;
  }

Calculating at a price of 105 USDC per OUSG:

amountE36 is calculated by scaling up the USDC amount:

$ \text{amountE36} = \text{_scaleUp}(1000000e6) \times 1e18 = 1000000000000000000000000e18 $.

ousgAmountOut is then determined by dividing by the price:

$ \text{ousgAmountOut} = \frac{1000000000000000000000000e18}{105e18} = 9523e18 $.

Bob is set to receive 9523 OUSG if the transaction is executed at a price of 105 USDC per OUSG.

Calculating at a price of 105.40 USDC per OUSG:

The amountE36 remains the same as the USDC amount hasn't changed.

$ \text{ousgAmountOut} = \frac{1000000000000000000000000e18}{105.40e18} = 9487e18 $.

Unfortunately, Bob will receive only 9487 OUSG instead of the expected 9523, which is fewer tokens than he would have received at the initial price of 105 USDC per OUSG. This is due to the price increase to 105.40 USDC per OUSG at the time of transaction execution, highlighting the impact of price volatility on minting operations without slippage protection.

Note:- mintRebasingOUSG, redeem,redeemRebasingOUSG, function also lacks slippage control.

Tools Used

Manual Review & Pen / paper

Recommended Mitigation Steps

Implement slippage protection in mint, mintRebasingOUSG, redeem, redeemRebasingOUSG functions.

Assessed type

Other

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as duplicate of #250

c4-pre-sort commented 6 months ago

0xRobocop marked the issue as duplicate of #156

c4-judge commented 6 months ago

3docSec marked the issue as satisfactory

c4-judge commented 6 months ago

3docSec changed the severity to QA (Quality Assurance)

c4-judge commented 6 months ago

3docSec marked the issue as grade-b

0xAbhay commented 6 months ago

@3docSec hey thanks for judging I believe this is medium big because, as you mentioned in finding #117, a price can be set more than 2% of the current price. However, even a 2% increase is too much to consider for institutional investors. This could lead to losses for the investors."

3docSec commented 6 months ago

this is medium big

opinion

a price can be set more than 2% of the current price

false, it's up to 2%

2% increase is too much to consider for institutional investors

opinion

A short reminder of our backstage guidelines: let's keep opinions out of the discussion. Please abstain from commenting unless you want to bring in new facts that were not considered.

0xAbhay commented 6 months ago

@3docSec Apologies for the breach of protocol earlier. To clarify, even a 0.38% increase can lead to substantial losses for users, as evidenced by the Proof of Concept (PoC) wherein a mere $0.40 increment resulted in users receiving fewer tokens than anticipated. This occurrence underscores the significance of even minor deviations from the expected price, as it directly impacts users' received quantities. Hence, maintaining precise control over price fluctuations is imperative to mitigate potential losses and ensure users' expectations are met.

3docSec commented 6 months ago

Noted

0xAbhay commented 6 months ago

@3docSec Thank you for acknowledging the judging work; I appreciate it. As I mentioned in my proof of concept (POC), the OUSG chart experienced a sudden price surge (even a <2% is also too much as i mentioned earlier ). If a user is utilizing any of these functions and the price increases during transaction execution, it could lead to losses for the user.

For example this can also be exploited by malicious node.

Nodes exploring the blockchain can identify this vulnerability by monitoring transactions involving the mint, mintRebasingOUSG, redeem, and redeemRebasingOUSG functions in the contract. They can observe instances where users interact with these functions and analyze the price of OUSG at the time of transaction confirmation. If the price of OUSG experiences sudden fluctuations during confirmation, nodes can detect potential losses for users due to the lack of a slippage mechanism.

Exploiting this vulnerability to cause user loss of funds involves executing transactions at times when the price of OUSG is volatile, particularly just before rapid surges or drops in price. By initiating transactions without slippage protection, users may receive fewer OUSG, rOUSG, or USDC tokens than anticipated, leading to financial losses.

For example, an attacker could monitor the OUSG price chart and identify moments of significant price movement. They could then execute transactions using the vulnerable functions, exploiting the lack of slippage protection to obtain tokens at a less favorable exchange rate than expected. This could result in users receiving fewer tokens than they anticipated, effectively causing them to lose funds compared to what they would have received with proper slippage protection in place. Thank you and will leave the final decision in your hand 🙏