[WP-H2] `NonUSTStrategy.sol` Improper handling of swap fees allows attacker to steal funds from other users

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/strategy/NonUSTStrategy.sol#L66-L69

NonUSTStrategy will swap the deposited non-UST assets into UST before depositing to EthAnchor. However, the swap fee is not attributed to the depositor correctly like many other yield farming vaults involving swaps (ZapIn).

An attacker can exploit it for the swap fees paid by other users by taking a majority share of the liquidity pool.

Root Cause

The swap fee of depositing is not paid by the depositor but evenly distributed among all users.

PoC

Given:

• A NonUST vault and strategy is created for FRAX;
• The liquidity in FRAX-UST curve pool is relatively small (<$1M).

The attacker can do the following:

1. Add $1M worth of liquidity to the FRAX-UST curve pool, get >50% share of the pool;
2. Deposit 1M FRAX to the vault, get a depositAmount of 1M;
3. The strategy will swap 1M FRAX to UST via the curve pool, paying a certain amount of swap fee;
4. Withdraw all the funds from the vault.
5. Remove the liquidity added in step 1, profit from the swap fee. (A majority portion of the swap fee paid in step 3 can be retrieved by the attacker as the attacker is the majority liquidity provider.)

If the vault happens to have enough balance (from a recent depositor), the attacker can now receive 1M of FRAX.

A more associated attacker may combine this with issue [WP-M4] and initiate a sandwich attack in step 3 to get even higher profits.

As a result, all other users will suffer fund loss as the swap fee is essentially covered by other users.

Recommendation

Consider changing the way new shares are issued:

1. Swap from Vault asset (eg. FRAX) to UST in deposit();
2. Using the UST amount out / total underlying UST for the amount of new shares issued to the depositor.

In essence, the depositor should be paying for the swap fee and slippage.

[0xBRM]

This is only an issue if we support low liquidity Curve pools We are also adding slippage control as per some other issue which would cause massive transfers using low liquidity pools to revert, fully mitigating this. Likelihood of this happening would also be quite low given that profitability would go down tremendously as curve LPs would move to that pool in order to capture higher base fees, dissuading the attacker from continuing.

That being said, I do agree that the curve swap fee (0.04%) should be paid by each individual depositor.

[dmvt]

This requires a number of external factors to line up just right. It is a medium risk according to the definition provided by Code4rena.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

[naps62]

Fixed in https://github.com/lindy-labs/sc_solidity-contracts/pull/69