StaticATokenLM::_claimRewardsOnBehalf: wrong update of _unclaimedRewards[onBehalfOf] if reward > totBal lead to user lose of pending rewards.

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/aave/StaticATokenLM.sol#L461-L482

Vulnerability details

Description

If for some reason the current contract reward token balance is lower than the rewards meant to be paid to onBehalf address, then this rewards can never be claimed.

    function _claimRewardsOnBehalf(
        address onBehalfOf,
        address receiver,
        bool forceUpdate
    ) internal {
        if (forceUpdate) {
            _collectAndUpdateRewards();
        }

        uint256 balance = balanceOf(onBehalfOf);
        uint256 reward = _getClaimableRewards(onBehalfOf, balance, false); // @audit unclaimed + pending rewards
        uint256 totBal = REWARD_TOKEN.balanceOf(address(this));

        if (reward > totBal) {
            reward = totBal; // @audit the idea here is to end up paying current rewards balance if the rewards to pay are greater. However, this also mean that there is still some unclaimed rewards pending to pay in a future. Current code does not take into account this.
        }
        if (reward > 0) {
            _unclaimedRewards[onBehalfOf] = 0; // @audit This lines assumes that reward <= totBal always, something that is not true given previous conditional block 
            _updateUserSnapshotRewardsPerToken(onBehalfOf);
            REWARD_TOKEN.safeTransfer(receiver, reward);
        }
    }

Impact

Lost of expected rewards

POC

1. Alice try to claim her unclaimed rewards through StaticATokenLM::claimRewardsToSelf, which are supposed to be 100 aReward tokens
2. For some reason REWARD_TOKEN.balanceOf(address(this)); returns 80 inside StaticATokenLM::_claimRewardsOnBehalf
3. Alice en up getting 80, and there is no way for her to claim her others 20 aReward tokens

Mitigation steps

    function _claimRewardsOnBehalf(
        address onBehalfOf,
        address receiver,
        bool forceUpdate
    ) internal {
        if (forceUpdate) {
            _collectAndUpdateRewards();
        }

        uint256 balance = balanceOf(onBehalfOf);
        uint256 reward = _getClaimableRewards(onBehalfOf, balance, false);
        uint256 totBal = REWARD_TOKEN.balanceOf(address(this));

+       if (reward == 0) {
+           return;
+       }

        if (reward > totBal) {
            reward = totBal;
+           _unclaimedRewards[onBehalfOf] -= reward;
-       }
+       } else {
+           _unclaimedRewards[onBehalfOf] = 0
+       }
+       _updateUserSnapshotRewardsPerToken(onBehalfOf);
+           REWARD_TOKEN.safeTransfer(receiver, reward);
-       if (reward > 0) {
-           _unclaimedRewards[onBehalfOf] = 0;
-           _updateUserSnapshotRewardsPerToken(onBehalfOf);
-           REWARD_TOKEN.safeTransfer(receiver, reward);
-       }
    }

Assessed type

Other

[c4-judge]

thereksfour marked the issue as duplicate of #10

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[c4-judge]

thereksfour marked the issue as partial-50

[thereksfour]

Compared to #10, lack of reasons for loss of rewards.