Tokens values of decimals are not justified in fee deduction

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/Common.sol#L657-L695

Vulnerability details

Impact

Loss of funds

Proof of Concept

SwapAndMint swaps the given path of tokens or token to the set of tokens:

Contract: V3Utils.sol

191:     function swapAndMint(SwapAndMintParams calldata params)  whenNotPaused() external payable returns (SwapAndMintResult memory result) {
192:         if (params.token0 == params.token1) {
193:             revert SameToken();
194:         }
195:         require(_isWhitelistedNfpm(address(params.nfpm)));
196:         IWETH9 weth = _getWeth9(params.nfpm, params.protocol);
197: 
198:         // validate if amount2 is enough for action
199:         if (params.swapSourceToken != params.token0 
200:             && params.swapSourceToken != params.token1
201:             && params.amountIn0 + params.amountIn1 > params.amount2
202:         ) {
203:             revert AmountError();
204:         }
205:         _prepareSwap(weth, params.token0, params.token1, params.swapSourceToken, params.amount0, params.amount1, params.amount2);
206:         SwapAndMintParams memory _params = params;
207: 
208:         DeductFeesEventData memory eventData;
209:         if (params.protocolFeeX64 > 0) {
210:             uint256 feeAmount0;
211:             uint256 feeAmount1;
212:             uint256 feeAmount2;
213:             // since we do not have the tokenId here, we need to emit event later
214:             (_params.amount0, _params.amount1, _params.amount2, feeAmount0, feeAmount1, feeAmount2) = _deductFees(DeductFeesParams(params.amount0, params.amount1, params.amount2, params.protocolFeeX64, FeeType.PROTOCOL_FEE, address(params.nfpm), 0, params.recipient, address(params.token0), address(params.token1), address(params.swapSourceToken)), false);
215: 
216:             eventData = DeductFeesEventData({
217:                 token0: address(params.token0),
218:                 token1: address(params.token1),
219:                 token2: address(params.swapSourceToken),
220:                 amount0: params.amount0,
221:                 amount1: params.amount1,
222:                 amount2: params.amount2,
223:                 feeAmount0: feeAmount0,
224:                 feeAmount1: feeAmount1,
225:                 feeAmount2: feeAmount2,
226:                 feeX64: params.protocolFeeX64,
227:                 feeType: FeeType.PROTOCOL_FEE
228:             });
229:         }
230:         result = _swapAndMint(_params, msg.value != 0);
231:         emit DeductFees(address(params.nfpm), result.tokenId, params.recipient, eventData);
232:     }

At L:205 the function calls _prepareSwap to convert msg.value to the chosen protocol´s WETH and pulls more funds if required. At L:214, the fees are deducted by passing the params, and _deductFees is called. And finally _swapAndMint is called at L: 230.

_swapAndMint function is below:

Contract: Common.sol

366:     function _swapAndMint(SwapAndMintParams memory params, bool unwrap) internal returns (SwapAndMintResult memory result) { 
367:         (uint256 total0, uint256 total1) = _swapAndPrepareAmounts(params, unwrap);
368:         
369:         if (params.protocol == Protocol.UNI_V3) {
370:             // mint is done to address(this) because it is not a safemint and safeTransferFrom needs to be done manually afterwards
371:             (result.tokenId,result.liquidity,result.added0,result.added1) = _mintUniv3(params.nfpm, univ3.INonfungiblePositionManager.MintParams(
372:                 address(params.token0),
373:                 address(params.token1),
374:                 params.fee,
375:                 params.tickLower,
376:                 params.tickUpper,
377:                 total0, 
378:                 total1,
379:                 params.amountAddMin0,
380:                 params.amountAddMin1,
381:                 address(this), // is sent to real recipient aftwards
382:                 params.deadline
383:             ));
384:         } else if (params.protocol == Protocol.ALGEBRA_V1) {
385:             // mint is done to address(this) because it is not a safemint and safeTransferFrom needs to be done manually afterwards
386:             (result.tokenId,result.liquidity,result.added0,result.added1) = _mintAlgebraV1(params.nfpm, univ3.INonfungiblePositionManager.MintParams( 
387:                 address(params.token0),
388:                 address(params.token1),
389:                 params.fee,
390:                 params.tickLower,
391:                 params.tickUpper,
392:                 total0, 
393:                 total1,
394:                 params.amountAddMin0,
395:                 params.amountAddMin1,
396:                 address(this), // is sent to real recipient aftwards
397:                 params.deadline
398:             ));
399:         } else {
400:             revert NotSupportedProtocol();
401:         }
402:         params.nfpm.transferFrom(address(this), params.recipient, result.tokenId);
403:         emit SwapAndMint(address(params.nfpm), result.tokenId, result.liquidity, result.added0, result.added1);
404:                 
405:         _returnLeftoverTokens(ReturnLeftoverTokensParams(_getWeth9(params.nfpm, params.protocol), params.recipient, params.token0, params.token1, total0, total1, result.added0, result.added1, unwrap));
406:     }

At L:367, _swapAndPrepareAmounts is called. _swapAndPrepareAmounts swaps the tokens to tokens by the input parameters. Accordingly, the conditions are as follows:

Contract: Common.sol

472:         if (params.swapSourceToken == params.token0) { 
479:         } else if (params.swapSourceToken == params.token1) { 
486:         } else if (address(params.swapSourceToken) != address(0)) {
503:         } else {
504:             total0 = params.amount0;
505:             total1 = params.amount1;
506:         }

As can be seen at L:486, if the swapSourceToken is not token0 and token1, it swaps the swapSourceToken to token0 and token1. And the fees are deducted from swapSourceToken accordingly.

However, this opens a vector for 2 conditions.

1. The swapSourceToken can be a worthless high decimal token like PEPE: So that the deducted fee does not add any value to the protocol
2. The swapSourceTokencan be an expensive one like WBTC (6 decimals) or GUSD (2 decimals): Accordingly, the fees for GUSD/PEPE or WBTC/PEPE swap would hit the user hard, being abnormal compared to other ones.

Tools Used

Manual Review

Recommended Mitigation Steps

If the protocol intends to have a rational fee over the amounts, we recommend implementing price checks over the Oracles. E.g., The swap fee is fixed on a 1 USD fee, then the 1 USD worth of ETH/WETH can be pulled during this execution

Assessed type

Other

[3docSec]

The report is of sufficient quality

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[Haupc]

We taking fee based on liquidity value of position. It doesn't matter if tokens in position is meme or high value token.

[3docSec]

The recommendation of a fixed fee is inconsistent with the protocol intent. Taking a fee from one of the swapped tokens (that may be worthless) is industry standard

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid