ge6a - Dos through large deposit

[sherlock-admin]

ge6a

high

Dos through large deposit

Summary

To maintain the balance between base tokens and side tokens, the Vault performs token swaps on Uniswap when rolling over epochs and during delta hedging (when minting/burning IG positions). The SwapAdapterRouter has a default slippage of 2% calculated on the price obtained from Chainlink. The problem is that when a large amount of tokens needs to be exchanged, the allowed slippage may be exceeded, leading to the impossibility of rolling over epochs and consequently causing a denial of service (DOS) on the protocol.

Vulnerability Detail

https://github.com/sherlock-audit/2024-02-smilee-finance/blob/3241f1bf0c8e951a41dd2e51997f64ef3ec017bd/smilee-v2-contracts/src/providers/SwapAdapterRouter.sol#L152-L205

The implementation of the slippage check can be seen in the functions SwapAdapterRouter.swapIn and SwapAdapterRouter.swapOut. It is important to determine the amount of tokens needed to exceed the maximum slippage. For this purpose, I used the swap tool in the Uniswap V3 app.

From the screenshots, it can be seen that $3M is needed for wETH, $1M for ARB, $645K for GMX, and only $30K for JOE. These are not excessively large amounts and it is entirely possible to reach them accidentally with a large deposit or with several smaller deposits in one epoch. On the other hand, a malicious user may deliberately deposit an appropriate amount to cause a DOS on the protocol. Even if the Vault is killed, deposits cannot be withdrawn through rescueShares because the requirement is for the Vault to be dead, which happens after the epoch is rolled over. There are two working solutions left to recover locked deposits and profits: 1) Administrators (or someone else) to deposit a certain amount of the opposite token directly to the Vault. This way, the amount that needs to be exchanged will be reduced so as not to exceed the maximum slippage. The problem is that after the epoch is rolled over, these funds will be distributed among share holders and the sender will lose them. 2) By increasing the maximum slippage through SwapAdapterRouter.setSlippage (the maximum is 10%) and rolling over the epoch at the cost of serious losses for LPs. This may not be enough to solve the problem, as is the case with JOE. It will be necessary to wait for the set delay in TimeLock to expire.

POC

This POC show how someone can make DOS using a single deposit but the same can be achieved with multiple deposits or withdraws. The amounts in this POC are for illustration and don't have any market meaning. To run this poc first should replace the content of testnet/TestnetSwapAdapter.sol with the one below. The idea of the changes is to simulate slippage. ```solidity // SPDX-License-Identifier: MIT pragma solidity ^0.8.15; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; import {IExchange} from "../interfaces/IExchange.sol"; import {IPriceOracle} from "../interfaces/IPriceOracle.sol"; import {ISwapAdapter} from "../interfaces/ISwapAdapter.sol"; import {AmountsMath} from "../lib/AmountsMath.sol"; import {SignedMath} from "../lib/SignedMath.sol"; import {TestnetToken} from "../testnet/TestnetToken.sol"; import {console} from "forge-std/console.sol"; contract TestnetSwapAdapter is IExchange, Ownable { using AmountsMath for uint256; // Variables to simulate swap slippage (DEX fees and slippage together) // exact swap slippage (maybe randomly set at test setup or randomly changed during test), has the priority over the range int256 internal _exactSlippage; // 18 decimals // range to control a random slippage (between min and max) during a swap (see echidna tests) int256 internal _minSlippage; // 18 decimals int256 internal _maxSlippage; // 18 decimals IPriceOracle internal _priceOracle; error PriceZero(); error TransferFailed(); error InvalidSlippage(); error Slippage(); constructor(address priceOracle) Ownable() { _priceOracle = IPriceOracle(priceOracle); } function setSlippage(int256 exact, int256 min, int256 max) external onlyOwner { if (min > max || SignedMath.abs(exact) > 1e18 || SignedMath.abs(exact) > 1e18 || SignedMath.abs(exact) > 1e18) { revert InvalidSlippage(); } _exactSlippage = exact; _minSlippage = min; _maxSlippage = max; } function changePriceOracle(address oracle) external onlyOwner { _priceOracle = IPriceOracle(oracle); } /// @inheritdoc IExchange function getOutputAmount(address tokenIn, address tokenOut, uint256 amountIn) external view returns (uint) { return _getAmountOut(tokenIn, tokenOut, amountIn); } /// @inheritdoc IExchange function getInputAmount(address tokenIn, address tokenOut, uint256 amountOut) external view returns (uint) { return _getAmountIn(tokenIn, tokenOut, amountOut); } /// @inheritdoc IExchange function getInputAmountMax(address tokenIn, address tokenOut, uint256 amountOut) external view returns (uint) { uint256 amountIn = _getAmountIn(tokenIn, tokenOut, amountOut); uint256 amountInSlip = slipped(amountIn, true); return amountIn > amountInSlip ? amountIn : amountInSlip; } /// @inheritdoc ISwapAdapter function swapIn(address tokenIn, address tokenOut, uint256 amountIn) external returns (uint256 amountOut) { if (!IERC20Metadata(tokenIn).transferFrom(msg.sender, address(this), amountIn)) { revert TransferFailed(); } amountOut = _getAmountOut(tokenIn, tokenOut, amountIn); amountOut = slipped(amountOut, false); (uint256 amountOutMin, uint256 amountOutMax) = _slippedValueOut(tokenIn, tokenOut, amountIn); if (amountOut < amountOutMin || amountOut > amountOutMax) { revert Slippage(); } TestnetToken(tokenIn).burn(address(this), amountIn); TestnetToken(tokenOut).mint(msg.sender, amountOut); } /// @inheritdoc ISwapAdapter function swapOut( address tokenIn, address tokenOut, uint256 amountOut, uint256 preApprovedAmountIn ) external returns (uint256 amountIn) { amountIn = _getAmountIn(tokenIn, tokenOut, amountOut); amountIn = slipped(amountIn, true); if (amountIn > preApprovedAmountIn) { revert InsufficientInput(); } (uint256 amountInMax, uint256 amountInMin) = _slippedValueIn(tokenIn, tokenOut, amountOut); if (amountIn < amountInMin || amountIn > amountInMax) { revert Slippage(); } if (!IERC20Metadata(tokenIn).transferFrom(msg.sender, address(this), amountIn)) { revert TransferFailed(); } TestnetToken(tokenIn).burn(address(this), amountIn); TestnetToken(tokenOut).mint(msg.sender, amountOut); } function _getAmountOut(address tokenIn, address tokenOut, uint amountIn) internal view returns (uint) { uint tokenOutPrice = _priceOracle.getPrice(tokenIn, tokenOut); amountIn = AmountsMath.wrapDecimals(amountIn, IERC20Metadata(tokenIn).decimals()); return AmountsMath.unwrapDecimals(amountIn.wmul(tokenOutPrice), IERC20Metadata(tokenOut).decimals()); } function _getAmountIn(address tokenIn, address tokenOut, uint amountOut) internal view returns (uint) { uint tokenInPrice = _priceOracle.getPrice(tokenOut, tokenIn); if (tokenInPrice == 0) { // Otherwise could mint output tokens for free (no input needed). // It would be correct but we don't want to contemplate the 0 price case. revert PriceZero(); } amountOut = AmountsMath.wrapDecimals(amountOut, IERC20Metadata(tokenOut).decimals()); return AmountsMath.unwrapDecimals(amountOut.wmul(tokenInPrice), IERC20Metadata(tokenIn).decimals()); } /// @dev "random" number for (testing purposes) between `min` and `max` function random(int256 min, int256 max) public view returns (int256) { uint256 rnd = block.timestamp; uint256 range = SignedMath.abs(max - min); // always >= 0 if (rnd > 0) { return min + int256(rnd % range); } return min; } /// @dev returns the given `amount` slipped by a value (simulation of DEX fees and slippage by a percentage) function slipped(uint256 amount, bool directionOut) public view returns (uint256) { int256 slipPerc = _exactSlippage; if(amount > 400000e18) { slipPerc = 0.21e18; } else return amount; int256 out; if (directionOut) { out = (int256(amount) * (1e18 + slipPerc)) / 1e18; } else { out = (int256(amount) * (1e18 - slipPerc)) / 1e18; } if (out < 0) { out = 0; } return SignedMath.abs(out); } function getSlippage(address tokenIn, address tokenOut) public view returns (uint256 slippage) { // Default baseline value: if (slippage == 0) { return 0.02e18; } } function _slippedValueOut( address tokenIn, address tokenOut, uint256 amountIn ) private view returns (uint256 amountOutMin, uint256 amountOutMax) { uint256 amountOut = _getAmountOut(tokenIn, tokenOut, amountIn); amountOutMin = (amountOut * (1e18 - getSlippage(tokenIn, tokenOut))) / 1e18; amountOutMax = (amountOut * (1e18 + getSlippage(tokenIn, tokenOut))) / 1e18; } function _slippedValueIn( address tokenIn, address tokenOut, uint256 amountOut ) private view returns (uint256 amountInMax, uint256 amountInMin) { uint256 amountIn = _getAmountIn(tokenIn, tokenOut, amountOut); amountInMax = (amountIn * (1e18 + getSlippage(tokenIn, tokenOut))) / 1e18; amountInMin = (amountIn * (1e18 - getSlippage(tokenIn, tokenOut))) / 1e18; } } ``` Then should add the function below to test/Scenarios.t.sol. ```solidity function testDosWithDeposit() public { string memory scenarioName = "scenario_1"; bool isFirstEpoch = true; console.log(string.concat("Executing scenario: ", scenarioName)); string memory scenariosJSON = _getTestsFromJson(scenarioName); console.log("- Checking start epoch"); StartEpoch memory startEpoch = _getStartEpochFromJson(scenariosJSON); _checkStartEpoch(startEpoch, isFirstEpoch); address attacker = address(0x4); uint256 amount = 810000e18; TokenUtils.provideApprovedTokens(_admin, _vault.baseToken(), attacker, address(_vault), amount, vm); vm.startPrank(attacker); _vault.deposit(amount, attacker, 0); Utils.skipWeek(true, vm); vm.startPrank(_admin); //this call will revert due to slippage _dvp.rollEpoch(); } ```

Impact

Dos of the protocol, lock of funds which can be recovered at the cost of significant losses for administrators or LPs.

Code Snippet

Above

Tool used

Manual Review

Recommendation

It is obvious that the slippage check cannot be removed because it protects the capital of LPs. A check for max deposits per epoch could be added, which, if well-calculated, would solve the problem, but it would severely limit the protocol. Another option is to introduce a max deposit per transaction and to balance the vault on every deposit (or when a certain amount is exceeded), not just when rolling over epochs. This is the solution I recommend.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid, if such situation happens, it means admin has set incorrect parameters (max deposit, chosen bad dex or tokens etc.) This is also a known issue in the list of known issues in the docs, stating that only high liquidity tokens/pools must be chosen.

[gstoyanovbg]

Escalate

With all due respect, @panprog , I disagree with the reasons you highlighted for invalidating the report. This is why i open this issue to be reviewed further.

1) In the README file, the tokens with which Smilee will initially work are specified. The DEX which will be used is also mentioned - Uniswap V3.

>Initially Smilee will have as:
Base token: USDC
Side tokens: ETH, wBTC, ARB, GMX, JOE
Underlying DEX: Uniswap V3

Therefore, the sponsors have deemed that these tokens have pools in Uniswap V3 with sufficient liquidity according to their criteria. For the screenshots in my report, are used the pools in Uniswap V3 with the highest liquidity for each of the listed tokens. Therefore, I believe that the argument that the pools do not have sufficient liquidity is not correct.

2) Regarding the argument for administrator's mistake. I clarified above that the DEX and tokens are chosen by the sponsors, and this report does not discuss random tokens on a random DEX. The protocol has a variable that controls the maximum deposit, but this is the maximum deposit that all LPs can deposit in total (there is no deposit cap for a single call or user). If too small value is set as deposit cap, it would mean that the protocol would not be profitable enough. And values of 1-2 million $ for TVL for such a protocol type are insignificant. If we look at defillama, there are protocols similar to Smilee with two-digit sums in millions of TVL. Furthermore, the protocol will not be able to grow over time, which in itself creates a problem rather than solving one.

3) It is not mentioned in panprog's comment, but this line of known issues potentially could be used by someone as an argument to invalidate the report.

DEX SWAP: issues related to impacts of fees and slippage are known and mitigated

In my opinion, however, this report is not about a standard slippage and fee issue with Dex swap but about a problem arising from the way the protocol is structured. As I described in the report itself, the mitigation for slippage issues probably created this problem, but the solution is not related to its change.

From all said above i believe that this is a valid issue and think that should be High severity (High impact, High Likelihood).

P.S I just was notified that the screenshots from the original report are not visible, so i will try to import them here again:

[sherlock-admin2]

Escalate

With all due respect, @panprog , I disagree with the reasons you highlighted for invalidating the report. This is why i open this issue to be reviewed further.

1) In the README file, the tokens with which Smilee will initially work are specified. The DEX which will be used is also mentioned - Uniswap V3.

>Initially Smilee will have as:
Base token: USDC
Side tokens: ETH, wBTC, ARB, GMX, JOE
Underlying DEX: Uniswap V3

Therefore, the sponsors have deemed that these tokens have pools in Uniswap V3 with sufficient liquidity according to their criteria. For the screenshots in my report, are used the pools in Uniswap V3 with the highest liquidity for each of the listed tokens. Therefore, I believe that the argument that the pools do not have sufficient liquidity is not correct.

2) Regarding the argument for administrator's mistake. I clarified above that the DEX and tokens are chosen by the sponsors, and this report does not discuss random tokens on a random DEX. The protocol has a variable that controls the maximum deposit, but this is the maximum deposit that all LPs can deposit in total (there is no deposit cap for a single call or user). If too small value is set as deposit cap, it would mean that the protocol would not be profitable enough. And values of 1-2 million $ for TVL for such a protocol type are insignificant. If we look at defillama, there are protocols similar to Smilee with two-digit sums in millions of TVL. Furthermore, the protocol will not be able to grow over time, which in itself creates a problem rather than solving one.

3) It is not mentioned in panprog's comment, but this line of known issues potentially could be used by someone as an argument to invalidate the report.

DEX SWAP: issues related to impacts of fees and slippage are known and mitigated

In my opinion, however, this report is not about a standard slippage and fee issue with Dex swap but about a problem arising from the way the protocol is structured. As I described in the report itself, the mitigation for slippage issues probably created this problem, but the solution is not related to its change.

From all said above i believe that this is a valid issue and think that should be High severity (High impact, High Likelihood).

P.S I just was notified that the screenshots from the original report are not visible, so i will try to import them here again:

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Czar102]

@gstoyanovbg can't the team simply increase the slippage and if that's not enough, provide Just-In-Time liquidity? That should solve the problem, right?

Also, I think the line from the README you quoted does include slippage fundamentally being so large that the transaction fails, and that's out of scope.

Planning to reject the escalation and leave the issue as is.

[gstoyanovbg]

@Czar102

Of course, the administrator can increase the max slippage when a described Dos occurs, wait for the delay period to expire, and roll the epoch. Imagine 5 LPs depositing certain amounts that individually do not exceed the limit for triggering slippage protection but collectively surpass it. There is nothing irregular in the actions of these LPs, but nevertheless, a significant percentage of the amount will be lost at the accounting phase. This loss will not be distributed only among these 5 LPs but will be paid by all LPs. This is because in _beforeRollEpoch, sharesToMint is first calculated based on the total deposit, and only then adjustBalances is executed, resulting in a loss of part of the amount. In other similar protocols, the idea of delay similar to the one described is to prevent changes in the protocol parameters within one epoch. If this is true for Smilee (which there are reasons to believe it is), this is another argument against this scenario.

Providing JIT liquidity cannot be a long-term solution to the problem described in this report. Smilee plans to work with different tokens and Dex-es, which means that multiple vaults will be deployed. Such a DoS can occur frequently and on different vaults. For the team to be able to roll epochs through JIT liquidity, either a large amount of liquidity must be maintained or loans must be taken. Loans incur fees, meaning a loss of funds for the team. Maintaining a sufficient amount of each token is not practical and would limit the protocol's development potential. If a certain amount is held in stable coins, a swap will have to be made to provide JIT liquidity, which again incurs fees and loss of funds. The README file states that Smilee uses an off-chain scheduler to roll epochs, so this bot will not be able to function correctly all the time.

To ensure better profits for the LPs compared to Uniswap, Smilee uses solid mathematics. It does not take into consideration significant losses from excessively large slippage or periods in which the epoch cannot be rolled, and consequently, no trading fees are collected. In other words, the main feature of the protocol is broken because Smilee may underperform compared to Uniswap leading to loss of funds for the LPs in Smilee.

I would agree with your interpretation of the line i quoted if this report was about a bug of the type - "this operation may cause slippage and slippage protection needs to be added". The fix for this report is not related to adding slippage protection anywhere, and I don't see the logic in invalidating it because of the discussed quote. In support of this claim, I want to emphasize that the documentation attached in the README file includes a table describing known financial bugs to the team. The problem described in this report is not in the list, and in my opinion, we are not dealing with a trivial problem compared to those described there. I want to draw attention to the fact that in this list, there are several problems whose mitigation involves max slippage protection, which means that increasing it will put users at risk.

[Czar102]

@gstoyanovbg I think the issues you are mentioning are noted in this table.

Smilee vault may underperform DEX LPs due to DEX slippage and fees when swapping

Liquidity on the DEX pool decrease significantly [causing delta hedge or roll-epoch to revert due to max slippage]

It seems the issue is already known to the team, and hence I'm planning to reject the escalation and leave the issue as is.

[gstoyanovbg]

@Czar102

Smilee vault may underperform DEX LPs due to DEX slippage and fees when swapping

Look at how this known issue is mitigated. It's obvious that the cause for it was the lack of slippage protection to limit losses from slippage and fees. It has been added, thus solving this problem. This was exactly what I meant when I said in my previous comment that the team cannot arbitrarily increase the maximum slippage because it will reopen this resolved issue. Accordingly, this was in response to your comment that it is possible to simply increase the slippage. The root cause of this problem and the problem I am describing in this report are unrelated, so we cannot say it is the same problem. The quoted statement itself can be used for multiple bugs leading to this result, each different from the others. However, the context provided by the description and the way it is fixed clearly shows exactly which problem has been addressed.

Liquidity on the DEX pool decrease significantly [causing delta hedge or roll-epoch to revert due to max slippage]

In this report, I am not talking about such an extreme situation, and I clearly showed that such a revert can occur at any time due to the way deposits are processed. Let's look at the assessment column:

Limited impact because the max slippage on swap protects the vault from excessive slippage on the swap. If liquidity does not come back to the DEX, the issue is probably more related to the DEX / token itself. A new DEX venue can be found in case or the vault can be paused and killed

This description has absolutely nothing to do with the problem I am describing, and therefore, neither does the fix. Therefore, I do not understand how this known issue invalidates my report.

Throughout the document, I do not see anywhere mentioned that the situation described in the report may occur, that this problem was intentionally not addressed, or that it was mitigated. Some of the described issues may, due to their general formulation, resemble this report, but in other discussions in Sherlock, I have seen that the root cause of the problem should be considered and also how it can be fixed. In my opinion, the same approach should be taken in this situation as well.

This general comment is valid for report #142 as well.

[Czar102]

I understand that this report doesn't talk about a situation when the liquidity is decreased, and the situation described may happen no matter how deep the liquidity is, but requires a larger deposit in case the liquidity is deeper. I still think this issue is covered within the mentioned point.

Even if it wasn't, it is invalid because all slippage-related issues were taken out of scope, and this issue focuses on slippage.

I'm still planning to reject the escalation and leave the issue as is.

[gstoyanovbg]

This is my last comment in this discussion.

[Czar102]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• gstoyanovbg: rejected