sherlock-audit / 2023-02-gmx-judging

17 stars 11 forks source link

berndartmueller - The claimable collateral factor with the key `Keys.claimableCollateralFactorKey` remains unchanged and results in a claimable collateral amount of zero #196

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

berndartmueller

medium

The claimable collateral factor with the key Keys.claimableCollateralFactorKey remains unchanged and results in a claimable collateral amount of zero

Summary

The MarketUtils.claimCollateral() is unable to calculate the claimable collateral amount due to the claimable factor (claimableFactor) not being updated/initialized and hence remains always equal to zero, resulting in an adjusted claimable amount (adjustedClaimableAmount) of zero. This, in turn, leads to the inability of users to claim collateral.

Vulnerability Detail

To calculate the claimable collateral amount, the MarketUtils.claimCollateral() function multiplies the claimable amount (claimableAmount) with the claimable factor (claimableFactor). The claimable factor is retrieved from the DataStore using the Keys.claimableCollateralFactorKey key, which depends on dynamic arguments timeKey and account, indicating that the value is calculated dynamically (and not set by governance).

However, the claimable factor is never adapted, and its value remains unchanged, always equal to zero, as no code is in place to update it.

Consequently, the adjusted claimable amount (adjustedClaimableAmount) is always zero, resulting in an inability for users to claim collateral.

Impact

Users are unable to claim collateral even if there is available collateral.

Code Snippet

contracts/market/MarketUtils.sol#L631

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)); // @audit-info this key is dependent on very dynamic arguments `timeKey` and `account`. It's never changed and remains zero
629:     uint256 claimedAmount = dataStore.getUint(Keys.claimedCollateralAmountKey(market, token, timeKey, account));
630:
631:     uint256 adjustedClaimableAmount = Precision.applyFactor(claimableAmount, claimableFactor);
632:     if (adjustedClaimableAmount <= claimedAmount) {
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:     );
648:
649:     MarketEventUtils.emitCollateralClaimed(
650:         eventEmitter,
651:         market,
652:         token,
653:         timeKey,
654:         account,
655:         receiver,
656:         remainingClaimableAmount
657:     );
658: }

Tool used

Manual Review

Recommendation

Consider adjusting the implementation logic of the MarketUtils.claimCollateral function and add functionality to update the claimable collateral factor with the key Keys.claimableCollateralFactorKey.

Duplicate of #137