M-19. Inaccurate Fee Due to missing lastFeeCollected Update Before feePerSecond Modification

Submitted by rvierdiiev, 0xCiphky, pontifex. Selected submission by: 0xCiphky.

Relevant GitHub Links

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXVault.sol#L334

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXVault.sol#L615

Summary

The protocol charges a management fee based on the feePerSecond variable, which dictates the rate at which new vault tokens are minted as fees via the mintFee function. An administrative function updateFeePerSecond allows the owner to alter this fee rate. However, the current implementation does not account for accrued fees before the update, potentially leading to incorrect fee calculation.

Vulnerability Details

The contract's logic fails to account for outstanding fees at the old rate prior to updating the feePerSecond. As it stands, the updateFeePerSecond function changes the fee rate without triggering a mintFee, which would update the lastFeeCollected timestamp and mint the correct amount of fees owed up until that point.

plain text
function updateFeePerSecond(uint256 feePerSecond) external onlyOwner {
_store.feePerSecond = feePerSecond;
emit FeePerSecondUpdated(feePerSecond);
}

Scenario Illustration:

User A deposits, triggering mintFee and setting lastFeeCollected to the current block.timestamp.

After two hours without transactions, no additional mintFee calls occur.

The owner invokes updateFeePerSecond to increase the fee by 10%.

User B deposits, and mintFee now calculates fees since lastFeeCollected using the new, higher rate, incorrectly applying it to the period before the rate change.

Impact

The impact is twofold:

An increased feePerSecond results in excessively high fees charged for the period before the update.

A decreased feePerSecond leads to lower-than-expected fees for the same duration.

Tools Used

Manual Analysis

Recommendations

Ensure the fees are accurately accounted for at their respective rates by updating lastFeeCollected to the current timestamp prior to altering the feePerSecond. This can be achieved by invoking mintFee within the updateFeePerSecond function to settle all pending fees first:

plain text
function updateFeePerSecond(uint256 feePerSecond) external onlyOwner {
self.vault.mintFee();
_store.feePerSecond = feePerSecond;
emit FeePerSecondUpdated(feePerSecond);
}