Submission Details

#13 Wrong Slippage Implementation

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/SmartVaultV3.sol#L206-L212

Summary

The code employs an on-chain slippage calculation mechanism through the calculateMinimumAmountOut function, utilized within the swap function. Consequently, this function may return 0 when the collateralization is deemed adequate.

        return collateralValueMinusSwapValue >= requiredCollateralValue ?
            0 : calculator.eurToToken(getToken(_outTokenSymbol), requiredCollateralValue - collateralValueMinusSwapValue);

Vulnerability Details

The absence of slippage checks and minimum return amount validations in the code could result in trades occurring at suboptimal prices, potentially leading to the reception of fewer tokens than would be expected at prevailing fair market rates. This vulnerability might expose the vault owner to risks of incurring losses due to unfavorable prices at the time of trade execution. Because the swap function will call ExactInputSingleParams function with amountOutMinimum set to 0:

    function swap(bytes32 _inToken, bytes32 _outToken, uint256 _amount) external onlyOwner {
        uint256 swapFee = _amount * ISmartVaultManagerV3(manager).swapFeeRate() / ISmartVaultManagerV3(manager).HUNDRED_PC();
        address inToken = getSwapAddressFor(_inToken);
        uint256 minimumAmountOut = calculateMinimumAmountOut(_inToken, _outToken, _amount);
        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
                tokenIn: inToken,
                tokenOut: getSwapAddressFor(_outToken),
                fee: 3000,
                recipient: address(this),
                deadline: block.timestamp,
                amountIn: _amount - swapFee,
                amountOutMinimum: minimumAmountOut,
                sqrtPriceLimitX96: 0
            });
        inToken == ISmartVaultManagerV3(manager).weth() ?
            executeNativeSwapAndFee(params, swapFee) :
            executeERC20SwapAndFee(params, swapFee);
    }

Impact

The risk associated with the absence of slippage and minimum return amount checks lies in potential price volatility during the swap. Trades can happen at a bad price and lead to receiving fewer tokens than at a fair market price.

Tools Used

Manual review.

Recommendations

Ensure that users are allowed to specify their own slippage parameters which were calculated on their own e.g off-chain.

Comments and Activity

Lead Judging Started

hrishibhat Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Slippage-issue

ljj Auditor
4 months ago

Should be invalid because this is a known issue in the README file. minimumAmountOut can be set to 0 if there is no value required to keep collateral above required level. The user is likely to lose some value in the swap, especially when Uniswap fees are factored in, but this is at the user's discretion. Shows no way of attacking that isn't at users discretion.

t0x1c Auditor
3 months ago

Finding 537 shows a real attack vector, which needs to be separated from this group of "Slippage-issue" and be considered in isolation, in my view.

tpiliposian Submitter
3 months ago

Shouldn’t be invalidated, as there can be a sandwich attack

hrishibhat Lead Judge
3 months ago

While this is true that the swap implementation allows for slippage as mentioned above, the contest page mentions loss of the user loss is to be expected and the swap function is only designed to check for value sufficient to keep it collateralized. The fair interpretation of this is that the limitations described above are known.

hrishibhat Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

While this is true that the swap implementation allows for slippage as mentioned above, the contest page mentions loss of the user loss is to be expected and the swap function is only designed to check for value sufficient to keep it collateralized. The same was confirmed by the sponsor. The fair interpretation of this is that the limitations described above are known.

Assigned finding tags:

Slippage-issue