c4-bot-5 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Utils.sol#L76-L107

Vulnerability details

Impact

Loss of protocol fees & funds

Proof of Concept

The fees are also passed in execute function as crafted inside the Instructions struct calldata:

Contract: V3Utils.sol

 76:     function execute(INonfungiblePositionManager _nfpm, uint256 tokenId, Instructions calldata instructions)  whenNotPaused() external
 77:     {
 78:         // must be approved beforehand
 79:         _nfpm.safeTransferFrom(
 80:             msg.sender,
 81:             address(this),
 82:             tokenId,
 83:             abi.encode(instructions)
 84:         );
 85:     }
 86: 
 87:     /// @notice ERC721 callback function. Called on safeTransferFrom and does manipulation as configured in encoded Instructions parameter. 
 88:     /// At the end the NFT (and any newly minted NFT) is returned to sender. The leftover tokens are sent to instructions.recipient.
 89:     function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)  whenNotPaused() external override returns (bytes4) {
 90:         INonfungiblePositionManager nfpm = INonfungiblePositionManager(msg.sender);
 91:         // not allowed to send to itself
 92:         if (from == address(this)) {
 93:             revert SelfSend();
 94:         }
 95: 
 96:         require(_isWhitelistedNfpm(address(nfpm)));
 97: 
 98:         Instructions memory instructions = abi.decode(data, (Instructions));
 99: 
100:         (address token0,address token1,,,,uint24 fee) = _getPosition(nfpm, instructions.protocol, tokenId);
101: 
102:         (uint256 amount0, uint256 amount1) = _decreaseLiquidityAndCollectFees(DecreaseAndCollectFeesParams(nfpm, instructions.recipient, IERC20(token0), IERC20(token1), tokenId, instructions.liquidity, instructions.deadline, instructions.amountRemoveMin0, instructions.amountRemoveMin1, instructions.compoundFees));
103: 
104:         // take protocol fees
105:  >      if (instructions.protocolFeeX64 > 0) { //@audit Fees can be bypassed by passing 0
106:             (amount0, amount1,,,,) = _deductFees(DeductFeesParams(amount0, amount1, 0, instructions.protocolFeeX64, FeeType.PROTOCOL_FEE, address(nfpm), tokenId, instructions.recipient, token0, token1, address(0)), true);
107:         }

So it´s possible to handcraft the fee as 0 in L:76 inside calldata

It will be executed in L: 105 if only it´s greater than 0

This leads to bypassing fees being deducted. And below snippet remains obsolete:

Contract: Common.sol

674:         if (params.feeX64 == 0) {
675:             revert NoFees();
676:         }

The same vulnerability is applicable to gas fees too:

Contract: V3Automation.sol

105:             if (params.gasFeeX64 > 0) { 
106:                 (,,, gasFeeAmount0, gasFeeAmount1,) = _deductFees(DeductFeesParams(state.amount0, state.amount1, 0, params.gasFeeX64, FeeType.GAS_FEE, address(params.nfpm), params.tokenId, positionOwner, state.token0, state.token1, address(0)), true);
107:             }

And since this one is originated by the OPERATOR, the execution gas fees will be taken from the operator without it´s being paid by the user.

Tools Used

Manual Review

Recommended Mitigation Steps

The check should be as below with a range of fee limits:

Contract: V3Utils.sol

- if (instructions.protocolFeeX64 > 0) { 
+ require (instructions.protocolFeeX64 > someLowerBoundaryHere) {

Contract: V3Automation.sol

- if (params.gasFeeX64 > 0) { 
+ require (params.gasFeeX64 > someLowerBoundaryOrCalculationHere) {