sherlock-audit / 2023-02-gmx-judging

17 stars 11 forks source link

berndartmueller - Inability to claim collateral #192

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

berndartmueller

high

Inability to claim collateral

Summary

Due to using the incorrect comparison operator in the MarketUtils.claimCollateral function, a user is unable to claim available collateral from a market.

Vulnerability Detail

In the MarketUtils.claimCollateral() function, the if statement in line 632 uses an inappropriate comparison operator. The used >= comparison operator asserts that the claimable amount (adjustedClaimableAmount) is greater than or equal to the claimed amount (claimedAmount) and reverts with the CollateralAlreadyClaimed custom error if this is the case.

For a user claiming collateral for the first time, the claimedAmount is zero, and any available claimable amount (adjustedClaimableAmount) is non-zero. As a result, the if statement in line 632 will always evaluate to true, reverting with the CollateralAlreadyClaimed error.

Impact

A user is unable to claim collateral from a market.

Code Snippet

contracts/market/MarketUtils.sol#L632

618: function claimCollateral(
619:     DataStore dataStore,
620:     EventEmitter eventEmitter,
621:     address market,
622:     address token,
623:     uint256 timeKey,
624:     address account,
625:     address receiver
626: ) internal {
627:     uint256 claimableAmount = dataStore.getUint(Keys.claimableCollateralAmountKey(market, token, timeKey, account));
628:     uint256 claimableFactor = dataStore.getUint(Keys.claimableCollateralFactorKey(market, token, timeKey, account));
629:     uint256 claimedAmount = dataStore.getUint(Keys.claimedCollateralAmountKey(market, token, timeKey, account));
630:
631:     uint256 adjustedClaimableAmount = Precision.applyFactor(claimableAmount, claimableFactor);
632:     if (adjustedClaimableAmount >= claimedAmount) { // @audit-info invalid comparison operator
633:         revert CollateralAlreadyClaimed(adjustedClaimableAmount, claimedAmount);
634:     }
635:
...      // [...]
658: }

Tool used

Manual Review

Recommendation

Consider adjusting the comparison operator in the if statement in line 632 to prevent claiming already claimed collateral:

if (adjustedClaimableAmount <= claimedAmount) {
    // [...]
}

Duplicate of #136