QA Report

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/UniV3LpVault.sol#L623-L625

Vulnerability details

Impact

There is no direct slippage control for the results of the swaps via UniV3LpVault's _swap function, so its calls are subject to sandwich attacks. Trades can happen at a manipulated price and end up receiving fewer tokens than current market price dictates.

Placing severity to medium as swapping is used in the system during a number of core operations (repayDebt, flashFocusCall, also compoundFees and moveRange via _prepareForDeposit), while funds in some of these cases can be substantial, so sandwich attacks are often enough economically viable and thus probable, while they result in a partial fund loss.

Proof of Concept

A range of functions invoke _swap, which takes in the path and input amount, and accept any output amount:

https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/UniV3LpVault.sol#L623-L625

_swap is used across the logic, including cases where amount can be substantial, which make sandwich attacks viable, for example repayDebt and flashFocusCall:

https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/UniV3LpVault.sol#L520-L521

https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/UniV3LpVault.sol#L379

Recommended Mitigation Steps

Consider adding minimum accepted return argument to the _swap and pass it through from mentioned user-facing functions and condition execution success on it. The user can always choose to leave the minimum empty, but there should be a way to control it at the level of total output amounts.

[0xdramaone]

Disagree that slippage protection is needed in the _swap function, as this is used for intermediary transactions where we have surrounding slippage checks (see params.amountMin in compoundFees, moveRange, flashFocus ). So long as we meet our minExpected amounts in our final entry of liquidity into a range, we have experienced bounded net slippage.

Low risk

[jack-the-pug]

Downgraded to QA as slippage protection is done already at a higher level.

[JeeberC4]

Generating QA Report as warden did not submit one and judge downgraded issue. Preserving original title: UniV3LpVault swaps can be subject to sandwich attacks as total output can't be user controlled