Reserve Protocol - Invitational - bin2chen's results

thereksfour marked the issue as primary issue

pmckelvy1 marked the issue as sponsor confirmed

pmckelvy1 marked the issue as sponsor acknowledged

thereksfour marked the issue as satisfactory

thereksfour marked the issue as selected for report

thereksfour marked the issue as primary issue

We have considered this very issue before and have decided as a solution to avoid raw ETH and ERC777's entirely, and not try to detect reentrancy in the way described in the article. This is for a few reasons:

1. It cannot be implemented uniformly. withdraw_admin_fees is not on all Curve pools, and in particular not on Tricrypto. https://etherscan.io/address/0xd51a44d3fae010294c616388b506acda1bfaae46
2. Raw ETH presents other challenges. Even if we detect reentrancy in the way suggested, the assetRegistry making multiple asset refresh() calls means an attacker could gain execution under a different asset's refresh() and use that to manipulate refPerTok().

Also related: Our target unit system does not work well with volatile pools. Each pool would need its own target unit and therefore could not be backed up with any other collateral except identically/distributed pools. We plan to remove CurveVolatileCollateral entirely.

There is documentation on the website indicating to avoid ERC777 tokens, but this should probably be updated to include forbidding LP tokens that contain raw ETH. Though, it is a bit more complicated than that since some LP tokens offer withdrawal functions that automate the unwrapping of WETH into ETH.

https://reserve.org/protocol/rtokens/#non-compatible-erc20-assets

pmckelvy1 marked the issue as sponsor confirmed

thereksfour marked the issue as satisfactory

thereksfour marked the issue as selected for report

Dup of https://github.com/code-423n4/2023-07-reserve-findings/blob/main/data/ronnyx2017-Q.md#7-curve-read-only-reentrancy-can-increase-the-price-of-some-curvestablecollateral . In fact The price manipulated mentioned in the https://github.com/code-423n4/2023-07-reserve-findings/issues/14 is Curve Read-Only Reentrancy attack. And I explained this attack in more detail in my QA report.

thereksfour marked the issue as not selected for report

thereksfour marked the issue as duplicate of #45

thereksfour marked the issue as primary issue

We will be reaching out to the Aave team to understand more about this. It seems there are multiple places in StaticATokenLM where reward steps are missing, and there may be reasons why.

We will mark this issue as Sponsor Acknowledged. It is true the situation described by the warden and that's the behavior we observe. However we will not be implementing any change in the code (besides adding some comments) for the following reasons:

• We do not expect rewards in Aave V2 to come back
• We checked with Aave and we believe the original reason for building it this way still holds, and that is for gas purposes. Even if for some reason rewards on AAve V2 come back the cost of updating the user rewards on every transfer outweighs the rewards that may be left "uncollected" after a transfer operation. It is important to remark that any deposit or withdraw done to the contract plus any call to collectRewards.., and any claim of rewards from the Reserve protocol, would setup the correct balances. So while it is true that transfers may in some cases not transfer rewards we expect this to only be slightly off.

To clarify this issue for users we will add a comment to the wrapper contract StaticATokenLM to clarify the situation with how rewards are handled on transfer.

tbrent marked the issue as sponsor acknowledged

thereksfour marked the issue as satisfactory

thereksfour marked the issue as selected for report

thereksfour marked the issue as primary issue

thereksfour changed the severity to 2 (Med Risk)

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

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.

tbrent marked the issue as sponsor disputed

@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);
    }

@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

_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();
    }

@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.

@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

Need review from sponsors. @julianmrodri

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)

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

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!

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

@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.

pmckelvy1 (sponsor) acknowledged

@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!

thereksfour marked the issue as satisfactory

thereksfour marked the issue as selected for report

thereksfour marked the issue as grade-a