search

code-423n4 / 2022-01-sandclock-findings

0 stars 0 forks source link

[WP-H3] `NonUSTStrategy.sol` A malicious user/attacker can game the system by `claimYield()` or `withdraw()` based on price changes #159

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

All the non-UST assets are converted to UST for investments, but the deposit amounts are recorded in Vault assets token (eg, BTC).

The current design/implementation of non-UST vaults makes it possible for attackers to profit no matter the price goes up or down, at the expense of other users.

PoC

Given:

If the price of BTC increases to $60,000:

The attacker can withdraw() and get back 10 BTC. (The Vault will use 0.6M UST to buy BTC and return to the attacker.)

If the price of BTC decreases to $30,000:

The attacker can claimYield() and claim 5 BTC. (The Vault now believes that the attacker is entitled to 5 BTC of yield.)

This can be amplified with:

  1. Faster turnover rate (no need to wait for 50% of price change, 1% of price change is good enough);
  2. More positions (open a lot of positions among all the non-UST Vaults at various prices);

Recommendation

Consider making NonUSTStrategy not swapping to and investing in UST, but investing in assets that are pegged to or based on the Vault asset, take BTC for example, the Strategy should be investing in ibBTC or other BTC based investments.

naps62 commented 2 years ago

Our goal is to support only stable coins, so I don't think this is an issue. @ryuheimat ?

r2moon commented 2 years ago

yeah, we support stable coins only,

dmvt commented 2 years ago

Given that the sponsor will only be supporting stablecoins, this issue should be mitigated. However, this is not made clear in the comments present on the strategy in question. I recommend adding a warning statement to the contract comments and documentation so that anyone who may take over this project later or fork it understands the original intention. Changing this to a low risk per Code4rena definitions.

1 — Low: Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments.