`claim` inside `LendingLedger` will revert if market removed from whitelist

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-neobase/blob/main/src/LendingLedger.sol#L122

Vulnerability details

Impact

If a market is removed from the whitelist before a user claims the reward, users cannot claim it, and the reward will remain stuck.

Proof of Concept

When users want to claim the reward from LendingLedger by calling claim, it will trigger update_market first before contract sending the reward to users.

https://github.com/code-423n4/2024-03-neobase/blob/main/src/LendingLedger.sol#L122

    function claim(address _market) external {
>>>     update_market(_market); // Checks if the market is whitelisted
        MarketInfo storage market = marketInfo[_market];
        UserInfo storage user = userInfo[_market][msg.sender];
        int256 accumulatedCanto = int256((uint256(user.amount) * market.accCantoPerShare) / 1e18);
        int256 cantoToSend = accumulatedCanto - user.rewardDebt;

        user.rewardDebt = accumulatedCanto;

        if (cantoToSend > 0) {
            (bool success, ) = msg.sender.call{value: uint256(cantoToSend)}("");
            require(success, "Failed to send CANTO");
        }
    }

Inside update_market, if the market is not whitelisted, the call will revert.

https://github.com/code-423n4/2024-03-neobase/blob/main/src/LendingLedger.sol#L65

    function update_market(address _market) public {
>>>     require(lendingMarketWhitelist[_market], "Market not whitelisted");
        MarketInfo storage market = marketInfo[_market];
        if (block.number > market.lastRewardBlock) {
            uint256 marketSupply = lendingMarketTotalBalance[_market];
            if (marketSupply > 0) {
                uint256 i = market.lastRewardBlock;
                while (i < block.number) {
                    uint256 epoch = (i / BLOCK_EPOCH) * BLOCK_EPOCH; // Rewards and voting weights are aligned on a weekly basis
                    uint256 nextEpoch = epoch + BLOCK_EPOCH;
                    uint256 blockDelta = Math.min(nextEpoch, block.number) - i;
                    // May not be the exact time, but will ensure that it is equal for all users and epochs.
                    // If this ever drifts significantly, the average block time and / or reference block time & number can be updated. However, update_market needs to be called for all markets beforehand.
                    uint256 epochTime = referenceBlockTime +
                        ((block.number - referenceBlockNumber) * averageBlockTime) /
                        1000;
                    market.accCantoPerShare += uint128(
                        (blockDelta *
                            cantoPerBlock[epoch] *
                            gaugeController.gauge_relative_weight_write(_market, epochTime)) / marketSupply
                    );
                    market.secRewardsPerShare += uint128((blockDelta * 1e36) / marketSupply); // Scale by 1e18, consumers need to divide by it
                    i += blockDelta;
                }
            }
            market.lastRewardBlock = uint64(block.number);
        }
    }

This could lead to an issue, if the market is removed from the whitelist before users can claim the reward, they cannot claim it, and the reward will remain stuck inside LendingLedger.

Tools Used

Manual review

Recommended Mitigation Steps

Consider to allow users provide additional flag param when calling claim so they can claim reward without triggering update_market.

-   function claim(address _market) external {
+   function claim(address _market, bool updateMarket) external {
-       update_market(_market); // Checks if the market is whitelisted
+       if (updateMarket) {
+         update_market(_market); 
+       }
        MarketInfo storage market = marketInfo[_market];
        UserInfo storage user = userInfo[_market][msg.sender];
        int256 accumulatedCanto = int256((uint256(user.amount) * market.accCantoPerShare) / 1e18);
        int256 cantoToSend = accumulatedCanto - user.rewardDebt;

        user.rewardDebt = accumulatedCanto;

        if (cantoToSend > 0) {
            (bool success, ) = msg.sender.call{value: uint256(cantoToSend)}("");
            require(success, "Failed to send CANTO");
        }
    }

Assessed type

Invalid Validation

[c4-judge]

MarioPoneder marked the issue as primary issue

[OpenCoreCH]

Technically true, but not really a problem for us / by design. Removing from the whitelist (i.e. blacklisting) is a permissioned operation that should only be performed under exceptional circumstances (e.g. when the corresponding market is exploited). I am not sure if the suggestion would be better in such scenarios because an attacker (that may have already performed some sync calls) could then still claim

[c4-sponsor]

OpenCoreCH (sponsor) acknowledged

[MarioPoneder]

Blacklisting is intended by design, governance controlled and reversible. Therefore rewards are not irrecoverably locked.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-a