IllIllI - Users that have to claim collateral more than once for a time slot, may get the wrong total amount

[sherlock-admin]

IllIllI

medium

Users that have to claim collateral more than once for a time slot, may get the wrong total amount

Summary

Users that have to claim collateral more than once for a given time slot, may get the wrong total amount, because the amount claimed is incorrectly set

Vulnerability Detail

When letting a user claim his/her collateral, the code looks up the claimable amount, does an adjustment based on a factor, sends that amount to the user, then updates the remaining amount claimable. The code incorrectly sets the factor-adjusted total claimable amount as the amount claimed, rather than the claimable amount.

Impact

Accounting of the claimed amount will be wrong, and the user will get less collateral back than they deserve, in some cases.

Code Snippet

// File: gmx-synthetics/contracts/market/MarketUtils.sol : MarketUtils.claimCollateral()   #1

631 @>         uint256 adjustedClaimableAmount = Precision.applyFactor(claimableAmount, claimableFactor);
632            if (adjustedClaimableAmount <= claimedAmount) { // @audit fixed comparison operator as mentioned in a separate issue
633                revert CollateralAlreadyClaimed(adjustedClaimableAmount, claimedAmount);
634            }
635    
636 @>         uint256 remainingClaimableAmount = adjustedClaimableAmount - claimedAmount;
637    
638            dataStore.setUint(
639                Keys.claimedCollateralAmountKey(market, token, timeKey, account),
640 @>             adjustedClaimableAmount
641            );
642    
643            MarketToken(payable(market)).transferOut(
644                token,
645                receiver,
646                remainingClaimableAmount
647:           );

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L631-L647

• A user triggers claimable collateral for 1 Eth (claimableAmount = 1)
• A keeper sets claimableFactor to 1.0
• The user calls claim, and gets the full 1 Eth, and claimedAmount becomes 1 (adjustedClaimableAmount)
• A keeper sets claimableFactor to 0.5 for that time slot
• The user triggers more claimable collateral for 1 Eth (claimableAmount = 2) for the same time slot
• The user calls claim. adjustedClaimableAmount is 2 * 0.5 = 1, remainingClaimableAmount is 1 - 1 = 0, so the user can't claim anything

The user should have been able to claim a total of 1.5 Eth, but was only able to claim the original 1 Eth, and then nothing more.

Tool used

Manual Review

Recommendation

diff --git a/gmx-synthetics/contracts/market/MarketUtils.sol b/gmx-synthetics/contracts/market/MarketUtils.sol
index 7624b69..3346296 100644
--- a/gmx-synthetics/contracts/market/MarketUtils.sol
+++ b/gmx-synthetics/contracts/market/MarketUtils.sol
@@ -637,7 +637,7 @@ library MarketUtils {
 
         dataStore.setUint(
             Keys.claimedCollateralAmountKey(market, token, timeKey, account),
-            adjustedClaimableAmount
+            claimableAmount
         );
 
         MarketToken(payable(market)).transferOut(

[xvi10]

the claimableFactor should be set only after the time slot for the timeKey has passed, there is no validation on the contract for this currently, this validation will be added if necessary, this feature is excluded from the current scope