Having gas fees dependent on amounts is not correct

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/V3Automation.sol#L102-L115

Vulnerability details

Impact

Loss of funds for user & protocol

Proof of Concept

The protocol collects Gas Fees for offline orders and currently the fees are calculated based on amounts as below:

Contract: V3Automation.sol

102:         {
103:             uint256 gasFeeAmount0;
104:             uint256 gasFeeAmount1;
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:             }
108:             uint256 protocolFeeAmount0;
109:             uint256 protocolFeeAmount1;
110:             if (params.protocolFeeX64 > 0) {
111:                 (,,, protocolFeeAmount0, protocolFeeAmount1,) = _deductFees(DeductFeesParams(state.amount0, state.amount1, 0, params.protocolFeeX64, FeeType.PROTOCOL_FEE, address(params.nfpm), params.tokenId, positionOwner, state.token0, state.token1, address(0)), true);
112:             }
113:             state.amount0 = state.amount0 - gasFeeAmount0 - protocolFeeAmount0;
114:             state.amount1 = state.amount1 - gasFeeAmount1 - protocolFeeAmount1;
115:         }

Accordingly, the fees are deducted from the positions and sent to the protocol address in order to use it during execution calls.

However, this flow is not correct due to there´s no logic behind gas price calculation.

If the deducted amount is greater than the gas to spend, it´s not returned to the user anywhere in the code. E.g.;

1. Alice has a position of 10_000 DAI
2. She wants to have an AUTO_COMPOUND action and accordingly set her offline order
3. The gas fee is 1% and it deducts 100 DAI from her position
4. The execution costs 25 USD in Ethereum Mainnet
5. 75 USD is not returned to Alice at the end of the V3Automation::execute function

AND

If 1% of the amounts are deducted from the user amounts, it´s not a guaranteed execution no matter if the call to this function succeeds.

In such situation, where the deducted amount is not covering the gas cost, the call will either fail or be spent from the OPERATOR account.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the traditional approach where the function´s gas consumption is calculated as:

uint256 firstGas = gasLeft();
// .... do all the executions here ..//
uint256 lastGas = gasLeft();
uint256 gasCons = firstGas - lastGas;

And finally query the network with GASPRICE opcode and see whether the provided funds are sufficient to cover the required gas after calculating with all these params.

Assessed type

Other

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[3docSec]

The report is of sufficient quality

[Haupc]

We calculate gas fee off chain to decide run the execution or not. Since gas is not take from native token, we can not use gasLeft() or GASPRICE to calculate gas onchain

[3docSec]

Intended behavior: it seems to me that like with regular transactions, the gas stipend is a bid; it's up to the executor to simulate off-chain, evaluate profitability, and prioritize execution of the most profitable transactions first, leaving the unprofitable in the pool

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid

[deliriusz]

Hi @3docSec,

We understand the sponsor and the judge view on this issue. However, there might be cases where the gas fee, even when calculated correctly, may be unexpected on chain, especially with exotic tokens. In case of high demand for blockchain, like sh*tcoin craze, transactions might be delayed by hours due to demand.

Additionally, the transaction submission might be delayed by the Krystal backend for whatever reason. In this time, price of the tokens may either drop or increase significantly. So, when the protocol submits a transaction to mempool and then it stays there for prolonged time, the price changes may make the protocol overcharge or undercharge for transaction execution.

In case of swap source token price increase, outdated minAmountOut will be accepted, like for example increasing the price by 50% - there is no protection against that. So, high fee will be paid and the transaction won't fail, because minAmountOut will still be reached.

Additionally, the codebase does not show any proof for a refund in case of transferred value > gas fee.

[3docSec]

Hi @deliriusz,

on the unexpected delay point, the ExecuteParams struct contains all the terms that the user is willing to accept, and these include a deadline timestamp (enforced by uniswap) that the user can (and should) use to protect themselves from delay risks. Operators on the other hand can (and should) protect themselves by simulating the transaction before submission.

This other point:

Additionally, the codebase does not show any proof for a refund in case of transferred value > gas fee.

has been answered already with https://github.com/code-423n4/2024-06-krystal-defi-findings/issues/14#issuecomment-2207048490