The rewards might be locked inside the contract due to the reentrancy attack.

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/erc20/RewardableERC20.sol#L111

Vulnerability details

Impact

The stakers might lose some rewards and the rewards might be locked inside the contract forever.

Proof of Concept

In RewardableERC20, users can claim their accumulated rewards using claimRewards().

    function claimRewards() external nonReentrant {
        _claimAndSyncRewards();
        _syncAccount(msg.sender);
        _claimAccountRewards(msg.sender);
    }

It claims and sync rewards from the staking contract first and transfers the rewards to users. Also, it has a nonReentrant modifier to prevent possible reentrancy approaches.

Due to this nonReentrant modifier, the reentrancy attack is impossible with claimRewards() only but users can call deposit() or withdraw inside the hook.

As a result, the attackers might manipulate lastRewardBalance like the below.

1. We assume rewardToken is an ERC777 token with the _afterTokenTransfer hook.
2. An attacker calls claimRewards(). Then _claimAccountRewards() is called after syncing rewards from the staking contract.

    function _claimAccountRewards(address account) internal {
        uint256 claimableRewards = accumulatedRewards[account] - claimedRewards[account];

        emit RewardsClaimed(IERC20(address(rewardToken)), claimableRewards);

        if (claimableRewards == 0) {
            return;
        }

        claimedRewards[account] = accumulatedRewards[account];

        uint256 currentRewardTokenBalance = rewardToken.balanceOf(address(this));

        // This is just to handle the edge case where totalSupply() == 0 and there
        // are still reward tokens in the contract.
        uint256 nonDistributed = currentRewardTokenBalance > lastRewardBalance
            ? currentRewardTokenBalance - lastRewardBalance
            : 0;

        rewardToken.safeTransfer(account, claimableRewards); //@audit possible reentrancy

        currentRewardTokenBalance = rewardToken.balanceOf(address(this));
        lastRewardBalance = currentRewardTokenBalance > nonDistributed
            ? currentRewardTokenBalance - nonDistributed
            : 0;
    }

3. In _claimAccountRewards(), let's consider lastRewardBalance = 100, claimableRewards = 10.
4. Inside the hook, the attacker calls deposit() and _claimAndSyncRewards() will be called within _beforeTokenTransfer().
5. In _claimAndSyncRewards(), lastRewardBalance will be 100 as it isn't updated in _claimAccountRewards() yet.
   And balanceAfterClaimingRewards will be 100 - 10 + newlyClaimedRewards because claimableRewards = 10 was transfered already in _claimAccountRewards().
6. The main assumption is newlyClaimedRewards is positive after the second call of _claimAssetRewards within the same transaction. It's because claimRewards() calls this function.
   This assumption might be possible to happen if the reward contract has a minimum threshold of deposit amount and it transfers the rewards during the second _claimAssetRewards after the attacker meets the threshold condition by depositing more funds.
7. So if new rewards amount = 10, balanceAfterClaimingRewards = 90 + 10 = 100 and _rewardsPerShare won't be increased because balanceAfterClaimingRewards == _previousBalance although it has claimed new rewards.
8. After the hook, lastRewardBalance will be set to 100.
   As a result, 10 rewards will be locked inside the contract.

So there are 2 assumptions to make this attack possible.

• rewardToken is an ERC777 token. I think it's not so strong assumption because claimRewards() has a nonReentrant modifier already with this assumption.
• _claimAssetRewards() claims some rewards during the second call in the same transaction after new deposit and
  it might be possible during the implementation for existing virtual functions.

Tools Used

Manual Review

Recommended Mitigation Steps

_claimAndSyncRewards() and _claimAccountRewards() should have a nonReentrant modifier individually instead of claimRewards.

Assessed type

Reentrancy

[c4-judge]

thereksfour marked the issue as primary issue

[carlitox477]

This issue is mentioned in #41 as MorphoTokenisedDeposit::rewardTokenBalance behavior can be affected by reentrancy though RewardableERC20::_claimAssetRewards

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[thereksfour]

The attack is not profitable, consider QA.