brakeless - When actual executionFee is greater than expected executionFee, lossFee is calculated incorrectly and keepers are not fairly compensated

[sherlock-admin4]

brakeless

Medium

When actual executionFee is greater than expected executionFee, lossFee is calculated incorrectly and keepers are not fairly compensated

Summary

In the GasProcess::processExecutionFee() method, when executionFee > cache.userExecutionFee, there are two issues.

First, the lossFee calculation always results in 0, which will prevent the system from incrementing totalLossExecutionFee correctly.

Second, keepers are not compensated fairly for calling the transaction and will lose fee costs, as the cost to execute the transaction will be greater than the fees that they receive.

Vulnerability Detail

// GasProcess::processExecutionFee() (L17 - L41)
function processExecutionFee(PayExecutionFeeParams memory cache) external {
    uint256 usedGas = cache.startGas - gasleft();
    uint256 executionFee = usedGas * tx.gasprice;   // @audit executionFee calculated
    uint256 refundFee;
    uint256 lossFee;
    if (executionFee > cache.userExecutionFee) {
        executionFee = cache.userExecutionFee;    // @audit executionFee overwritten
        lossFee = executionFee - cache.userExecutionFee;  // @audit shouldn't this step occur prior to caching?
    } else {
        refundFee = cache.userExecutionFee - executionFee;
    }
    VaultProcess.transferOut(
        cache.from,
        AppConfig.getChainConfig().wrapperToken,
        address(this),
        cache.userExecutionFee
    );
    VaultProcess.withdrawEther(cache.keeper, executionFee);   // @audit cache.userExecutionFee < executionFee
    if (refundFee > 0) {
        VaultProcess.withdrawEther(cache.account, refundFee);
    }
    if (lossFee > 0) {   // @audit never triggered
        CommonData.addLossExecutionFee(lossFee);
    }
}

// CommonData::addLossExecutionFee() (L132 - L136)
function addLossExecutionFee(uint256 amount) external {
    Props storage self = load();
    self.totalLossExecutionFee += amount;
    emit LossExecutionFeeUpdateEvent(self.totalLossExecutionFee - amount, self.totalLossExecutionFee);
}

Incorrect lossFee calculation

GasProcess::processExecutionFee() is called by two-phase commit functions that are executed by a keeper in OrderFacet, PositionFacet, and StakeFacet .

This method is intended to pay out the keeper execution fee.

This method also pays either a refund to the account’s owner if the actual execution fee is less than the expected fee, or tracks additional amounts as a lossFee in case execution costs more than it should.

In the code snippet above, the executionFee is initially calculated based on how much gas was consumed in the transaction.

If the actual executionFee > expected executionFee, the executionFee memory variable is overwritten with the expected executionFee (cache.executionFee), which is passed in as input.

Then, the lossFee is calculated as executionFee - cache.userExecutionFee = cache.userExecutionFee - cache.userExecutionFee = 0.

As a result, the conditional at the bottom of the function body is never executed and totalLossExecutionFee is never incremented.

Keepers are not fairly compensated

In addition, keepers will be paid cache.executionFee, which is less than the actual executionFee amount.

Protocols that utilize keepers often top-up and reserve additional funds within the system for keeper fees.

Elfi expects that users will pay for the executionFee most of the time, which is provided when creating an order or updating a position.

However, if the executionFee costs more than what is expected, keepers will pay more to execute the transaction than what they receive in fees, which may discourage keepers from doing work for the protocol.

Impact

totalLossExecutionFee is never updated or modified, as intended.

As a result, totalLossExecutionFee will remain 0 for the lifetime of the system.

Keepers will also lose fee costs when the execution costs more than expected.

Code Snippet

https://github.com/sherlock-audit/2024-05-elfi-protocol/blob/main/elfi-perp-contracts/contracts/process/GasProcess.sol#L17-L41

Tool used

Manual Review

Recommendation

The keeper should receive the actual executionFee every time.

Also, removing the caching in the first conditional block will allow lossFee to be calculated correctly and CommonData::addLossExecutionFee() to be executed as intended.

function processExecutionFee(PayExecutionFeeParams memory cache) external {
    uint256 usedGas = cache.startGas - gasleft();
    uint256 executionFee = usedGas * tx.gasprice;   
    uint256 refundFee;
    uint256 lossFee;
    if (executionFee > cache.userExecutionFee) {
-       executionFee = cache.userExecutionFee;
        lossFee = executionFee - cache.userExecutionFee;  
        
+       VaultProcess.transferOut(
+           cache.from,
+           AppConfig.getChainConfig().wrapperToken,
+           address(this),
+           executionFee
+       );
    } else {
        refundFee = cache.userExecutionFee - executionFee;
    
+       VaultProcess.transferOut(
+           cache.from,
+           AppConfig.getChainConfig().wrapperToken,
+           address(this),
+           cache.userExecutionFee  // cache.userExecutionFee = executionFee + refundFee
+       );
    }

    VaultProcess.withdrawEther(cache.keeper, executionFee);  // keeper always receives actual executionFee
    if (refundFee > 0) {
        VaultProcess.withdrawEther(cache.account, refundFee);
    }
    if (lossFee > 0) {   
        CommonData.addLossExecutionFee(lossFee);
    }
}

Duplicate of #108