The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

No slippage protection on token swaps may result in unfavorable outcomes for users

spacelord47

Summary

SmartVaultV3.swap function utilizes exactInputSingle swap-function from Uniswap, but doesn't provide slippage tolerance option for users.

Issue Details

Protocol users(borrowers) able to deposit ERC20 tokens to their vaults and later perform swaps on these tokens (inside the vault). Under the hood, the SmartVaultV3.swap function uses UniswapV3 pools.
In the following chunk of code we can see how swap parameters for Uniswap generated:

contracts/SmartVaultV3.sol

214: function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
...
217: uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
218: ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
219: tokenIn: inToken,
220: tokenOut: getSwapAddressFor(_outToken),
221: fee: 3000,
222: recipient: address(this),
223: deadline: block.timestamp,
224: amountIn: _amount - swapFee,
225: amountOutMinimum: minimumAmountOut,
226: sqrtPriceLimitX96: 0
227: });
...
231: }

And calculateMinimumAmountOut function, which calculates the amountOutMinimum value:

contracts/SmartVaultV3.sol

206: function calculateMinimumAmountOut(bytes32 _inTokenSymbol, bytes32 _outTokenSymbol, uint256 _amount) private view returns (uint256) {
207: ISmartVaultManagerV3 _manager = ISmartVaultManagerV3(manager);
208: uint256 requiredCollateralValue = minted * _manager.collateralRate() / _manager.HUNDRED_PC();
209: uint256 collateralValueMinusSwapValue = euroCollateral() - calculator.tokenToEur(getToken(_inTokenSymbol), _amount);
210: return collateralValueMinusSwapValue >= requiredCollateralValue ?
211: 0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);
212: }

At line 210 we can see that amountOutMinimum calculated only if user trying to swap tokens worth of more value than required collateral (requiredCollateralValue). This logic prevents the vault becoming undercollateralized after the swap. So, this logic protects the Protocol.

But if user performs a swap within the collateral bounds (standard case), then amountOutMinimum is set "0", which doesn't provide any slippage protection for user.

Impact

Missing slippage protection poses risks for users being "sandwiched" and receive much less funds than expected.

Front-running might be not be a big issue on Arbitrum One (where the Protocol currently operates), but Sponsor stated in the Contest details that the Protocol expected to be compatible with "Any EVM chains with live Chainlink data feeds and live Uniswap pools".

Recommendations

Consider adding a new param to the swap function, to allow users specify slippage tolerance (or at least set sensible defaults) .

Additionally, I'd suggest to simplify the logic which protects a position being undercollaterlized after swap:
instead of calculating amountOutMinimum in dangerously looking calculateMinimumAmountOut function, we can just perform swap (with amountOutMinimum provided by user), and then confirm that the position is healthy using existing undercollateralised function:

// @audit Existing function
99: function undercollateralised() public view returns (bool) {
100: return minted > maxMintable();
101: }
​
// @audit Updated "swap" function
function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount, uint256 _amountOutMinimum) external onlyOwner {
uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
address inToken = getSwapAddressFor(_inToken);
​
ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
tokenIn: inToken,
tokenOut: getSwapAddressFor(_outToken),
fee: 3000,
recipient: address(this),
deadline: block.timestamp,
amountIn: _amount - swapFee,
amountOutMinimum: _amountOutMinimum,
sqrtPriceLimitX96: 0
});
​
inToken == ISmartVaultManagerV3(manager).weth() ?
executeNativeSwapAndFee(params, swapFee) :
executeERC20SwapAndFee(params, swapFee);
​
// @audit Make sure the position remains healthy after the swap
require(!undercollateralised(), UNDER_COLL);
}
Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

Slippage-issue

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

Slippage-issue

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!