_claimRewardsOnBehalf() User's rewards may be lost

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/e3d2681503499e81915797c77eeef8210352a138/contracts/plugins/assets/aave/StaticATokenLM.sol#L459-L461

Vulnerability details

Impact

Incorrect determination of maximum rewards, which may lead to loss of user rewards

Proof of Concept

_claimRewardsOnBehalf() For users to retrieve rewards

    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 > totBal) {
@>          reward = totBal;
@>      }
        if (reward > 0) {
@>          _unclaimedRewards[onBehalfOf] = 0;
            _updateUserSnapshotRewardsPerToken(onBehalfOf);
            REWARD_TOKEN.safeTransfer(receiver, reward);
        }
    }

From the code above, we can see that if the contract balance is not enough, it will only use the contract balance and set the unclaimed rewards to 0: _unclaimedRewards[user]=0.

But using the current contract's balance is inaccurate, REWARD_TOKEN may still be stored in `INCENTIVES_CONTROLLER

_updateRewards() and _updateUser(), are just calculations, they don't transfer REWARD_TOKEN to the current contract, but _unclaimedRewards[user] is always accumulating

1. _updateRewards() not transferable REWARD_TOKEN

   
   function _updateRewards() internal {
   ...
       if (block.number > _lastRewardBlock) {
   ...
   
           address[] memory assets = new address[](1);
           assets[0] = address(ATOKEN);

@> uint256 freshRewards = INCENTIVES_CONTROLLER.getRewardsBalance(assets, address(this)); uint256 lifetimeRewards = _lifetimeRewardsClaimed.add(freshRewards); uint256 rewardsAccrued = lifetimeRewards.sub(_lifetimeRewards).wadToRay();

@> _accRewardsPerToken = _accRewardsPerToken.add( (rewardsAccrued).rayDivNoRounding(supply.wadToRay()) ); _lifetimeRewards = lifetimeRewards; } }

2. but `_unclaimedRewards[user]` always accumulating

```solidity
    function _updateUser(address user) internal {
        uint256 balance = balanceOf(user);
        if (balance > 0) {
            uint256 pending = _getPendingRewards(user, balance, false);
@>          _unclaimedRewards[user] = _unclaimedRewards[user].add(pending);
        }
        _updateUserSnapshotRewardsPerToken(user);
    }

This way if _unclaimedRewards(forceUpdate=false) is executed, it does not trigger the transfer of REWARD_TOKEN to the current contract. This makes it possible that _unclaimedRewards[user] > REWARD_TOKEN.balanceOf(address(this)) According to the _claimedRewardsOnBehalf() current code, the extra value is lost.

It is recommended that if (reward > totBal) be executed only if forceUpdate=true, to avoid losing user rewards.

Tools Used

Recommended 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 > totBal) {
+       if (forceUpdate && reward > totBal) {
            reward = totBal;
        }
        if (reward > 0) {
            _unclaimedRewards[onBehalfOf] = 0;
            _updateUserSnapshotRewardsPerToken(onBehalfOf);
            REWARD_TOKEN.safeTransfer(receiver, reward);
        }
    }

Assessed type

Context

[c4-judge]

thereksfour marked the issue as primary issue

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[tbrent]

See https://github.com/code-423n4/2023-07-reserve-findings/issues/12#issuecomment-1670267488

[julianmrodri]

After a thorough review we can confirm this is not an issue. This is the way it should work and that's the reason why there is a forceUpdate param. When forceUpdate == true, then you will always have the latest rewards to claim and the updated balance.

When is set to false, it will only distribute the rewards that were previously collected (the ones available in the contract). It is correct there might be additional rewards to be collected, but that can easily be done with another call to the same function using the forceUpdate == true.

There are no rewards "lost" in the process, no fix needs to be implemented. Even though unclaimedRewards is set to zero, then it will be populated with all the pending rewards again so the amount will be ok.

Moreover, the suggested fix would brick the function most of the time (as usually rewards are bigger than balance because it includes uncollected but pending rewards), and in that case it would attempt to transfer rewards not available in the contract. The check of just sending the balance in those cases is required.

[c4-sponsor]

tbrent marked the issue as sponsor disputed

[thereksfour]

@bin2chen66 please take a look. It seems that since _getPendingRewards has a false parameter, _unclaimedRewards does not accumulate unclaimed rewards in the controller, so the rewards are not lost.

    function _updateUser(address user) internal {
        uint256 balance = balanceOf(user);
        if (balance > 0) {
            uint256 pending = _getPendingRewards(user, balance, false);
            _unclaimedRewards[user] = _unclaimedRewards[user].add(pending);
        }
        _updateUserSnapshotRewardsPerToken(user);
    }

[bin2chen66]

@thereksfour getPendingRewards(fresh = true) It doesn't matter if fresh istrue or false, because this can only be used to calculate the latest global accRewardsPerToken

Since _updateRewards() must be executed before _updateUser (user) is executed to ensure that accRewardsPerToken is up-to-date, it does not matter whether fresh is true

But the message above

Even though unclaimedRewards is set to zero,
 then it will be populated with all the pending rewards again so the amount will be ok.

It confuses me a bit, I might need to take another look I need to familiarize myself with this project again to see if I missed something

[thereksfour]

_getClaimableRewards returns _unclaimedRewards + pendingRewards, that is, reward = _unclaimedRewards + pendingRewards, so just setting _unclaimedRewards to 0 will not decrease pendingRewards, which may be somewhat helpful

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

        if (reward > totBal) {
            reward = totBal;
        }
        if (reward > 0) {
            _unclaimedRewards[onBehalfOf] = 0;
            _updateUserSnapshotRewardsPerToken(onBehalfOf);
            REWARD_TOKEN.safeTransfer(receiver, reward);
        }
...
    function _getClaimableRewards(
        address user,
        uint256 balance,
        bool fresh
    ) internal view returns (uint256) {
        uint256 reward = _unclaimedRewards[user].add(_getPendingRewards(user, balance, fresh));
        return reward.rayToWadNoRounding();
    }

[bin2chen66]

@thereksfour pendingRewards is assumed to be 0. But _unclaimedRewards[user] has a value, the point is that the value in there is not in the current contract, it's in INCENTIVES_CONTROLLER. If it's cleared, it's gone. I thank I need to take another look.

[bin2chen66]

@thereksfour I'll keep my original point Please help me see if I'm missing something. Thanks

The current implementation only moves rewards to the current contract if _collectAndUpdateRewards() is executed

_updateRewards() and _updateUser() are not triggered.

But _unclaimedRewards[user] is accumulated. _accRewardsPerToken and _userSnapshotRewardsPerToken[user] keeps getting bigger.

so that if no one has called _collectAndUpdateRewards() i.e. forceUpdate=false is not called.

This way the rewards balance in the contract will always be zero.

After _claimRewardsOnBehalf(forceUpdate=false). The user doesn't get any rewards, but _unclaimedRewards[user] is cleared to 0 and can't be refilled (note that it's not pendingRewards, assuming that pendingRewards is 0).

This way the rewards are lost

[thereksfour]

Need review from sponsors. @julianmrodri

[bin2chen66]

Here's a test case to look at Note:The balance of the current contract described above cannot be 0, it needs to be a little bit

add to StaticATokenLM.test.ts

    it('test_lost', async () => {
      const amountToDeposit = utils.parseEther('5')

      // Just preparation
      await waitForTx(await weth.deposit({ value: amountToDeposit.mul(2) }))
      await waitForTx(
        await weth.approve(staticAToken.address, amountToDeposit.mul(2), defaultTxParams)
      )

      // Depositing
      await waitForTx(
        await staticAToken.deposit(userSigner._address, amountToDeposit, 0, true, defaultTxParams)
      )
      await advanceTime(1);
      //***** need small reward balace
      await staticAToken.collectAndUpdateRewards()
      const staticATokenBalanceFirst = await stkAave.balanceOf(staticAToken.address);
      await advanceTime(60 * 60 * 24)

      // Depositing
      await waitForTx(
        await staticAToken.deposit(userSigner._address, amountToDeposit, 0, true, defaultTxParams)
      )

      const beforeRewardBalance = await stkAave.balanceOf(userSigner._address);
      const pendingRewardsBefore = await staticAToken.getClaimableRewards(userSigner._address)
      console.log("user Reward Balance(Before):",beforeRewardBalance);

      // user claim forceUpdate = false
      await waitForTx(await staticAToken.connect(userSigner).claimRewardsToSelf(false))

      const afterRewardBalance = await stkAave.balanceOf(userSigner._address);
      const pendingRewardsAfter = await staticAToken.getClaimableRewards(userSigner._address)
      console.log("user Reward Balance(After):",afterRewardBalance);

      const pendingRewardsDecline = pendingRewardsBefore.toNumber() - pendingRewardsAfter.toNumber() ;
      const getRewards= afterRewardBalance.toNumber() - beforeRewardBalance.toNumber() ;
      console.log("user pendingRewardsBefore:",pendingRewardsBefore);
      console.log("user pendingRewardsAfter:",pendingRewardsAfter);
      const staticATokenBalanceAfter = await stkAave.balanceOf(staticAToken.address);
      console.log("staticAToken Balance (before):",staticATokenBalanceFirst);
      console.log("staticAToken Balance (After):",staticATokenBalanceAfter);
      console.log("user lost:",pendingRewardsDecline - getRewards);

    })

$ yarn test:plugins:integration --grep "test_lost"

  StaticATokenLM: aToken wrapper with static balances and liquidity mining
Duplicate definition of RewardsClaimed (RewardsClaimed(address,address,uint256), RewardsClaimed(address,address,address,uint256))
    Rewards - Small checks
user Reward Balance(Before): BigNumber { value: "0" }
user Reward Balance(After): BigNumber { value: "34497547939" }
user pendingRewardsBefore: BigNumber { value: "1490345817336159" }
user pendingRewardsAfter: BigNumber { value: "34497293495" }
staticAToken Balance (before): BigNumber { value: "34497547939" }
staticAToken Balance (After): BigNumber { value: "0" }
user lost: 1490276822494725
      ✔ test_lost (947ms)

  1 passing (24s)

[julianmrodri]

Thanks for the example. I'll review and let you know

[julianmrodri]

Ok, the issue exists I can confirm now. This is a tricky one, nice catch. Found out that this was built this way on purpose. The idea is to allow the user to "sacrifice" some rewards for gas savings. You can see it in some of the tests, with comments like this one:

expect(pendingRewards5).to.be.eq(0) // User "sacrifice" excess rewards to save on gas-costs

We will discuss today what action we are taking with this issue.

in any case, the suggested mitigation does not address fully the issue, and causes the contract to fail under normal operations (simply try to run our test suite with that change). I believe we should probably address the main issue that the _unclaimed variable is set to 0, instead of to the rewards still pending to be collected.

What do you think about the function working this way? I ran a simple check and seems to address it at least for the example you provided. This mitigation was suggested on the other ticket linked here, it had some rounding issues but overall is the same.

  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.wadToRay();
        } else {
            _unclaimedRewards[onBehalfOf] = 0;
        }

        _updateUserSnapshotRewardsPerToken(onBehalfOf);
        REWARD_TOKEN.safeTransfer(receiver, reward);
    }

Thanks for taking a look!

[julianmrodri]

@thereksfour @bin2chen66 updated response above. Tagging you to make sure you are notified.

[bin2chen66]

@julianmrodri This one still has problems

1. If there is a pendingReward may underflow For example: balance = 10 _unclaimedRewards[user]=9 pendingRewards = 2

_unclaimedRewards[onBehalfOf] -= reward.wadToRay(); will underflow

2. finally executed _updateUserSnapshotRewardsPerToken(onBehalfOf); Then we need to accumulate pendingRewards to _unclaimedRewards[user].

Personally, I feel that if we don't want to revert, try this

  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) {
+          // Insufficient balance resulting in no transfers out put into _unclaimedRewards[]
+           _unclaimedRewards[onBehalfOf] = (reward -totBal).wadToRay();
            reward = totBal;
-           _unclaimedRewards[onBehalfOf] -= reward.wadToRay();
        } else {
            _unclaimedRewards[onBehalfOf] = 0;
        }

        _updateUserSnapshotRewardsPerToken(onBehalfOf);
        REWARD_TOKEN.safeTransfer(receiver, reward);
    }

This may still have this prompt. expect(pendingRewards5).to.be.eq(0) // User "sacrifice" excess rewards to save on gas-costs

But I feel that this use case should be changed. It is okay to sacrifice a little. If it is a lot, it is still necessary to prevent the user from executing it.

[c4-sponsor]

pmckelvy1 (sponsor) acknowledged

[julianmrodri]

@thereksfour @bin2chen66 We had a group call and we decided to ACKNOWLEDGE the issue but we will not make code changes, just add a comment in the contract explaining this risk of losing rewards if you call it with the false parameter.

The reasons are:

• We do not expect Aave V2 Rewards to come back in practice
• Our protocol is not exposed to this issue. Because in our code we always claim with the parameter set to true, and nobody can claim on behalf of the protocol with the false parameter, the Protocol will always get the latest rewards when claiming
• It is up to the user to call this with true or false. They might see value in calling it with false and sacrificing some rewards, which was the original reason it was built this way. But they always have the option to call it with the true parameter which will behave normally. So we consider it more like an option they have and not that much of a bug. We also do not expect people holding this wrapper token besides using it in our protocol.

However, we acknowledge and value the finding which was spot on and allowed us to understand the wrapper in more detail. Thanks for that!

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report