[ConcurRewardPool] Possible reentrancy when claiming rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol#L34

Vulnerability details

[ConcurRewardPool] Possible reentrancy when claiming rewards

Impact

Since the reward tokens are transferred before the balances are set to 0, it is possible to perform a reentrancy attack if the reward token has some kind of call back functionality e.g. ERC777. pBTC is an ERC777 token that is currently available on Convex. A similar attack occurred with imBTC on uniswap v1.

Proof of Concept

• Preparation
  1. Assume that pBTC is used as extra rewards for this victim convex pool.
  2. A malicious user interacts with Concur through a smart contract. He follows the standard flow and has some rewards to be claimed.
  3. The malicious user interacts with this smart contract to register a bad tokensToSend() callback function through the ERC-1820 contract.
  4. In this tokensToSend() function, he calls ConcurRewardPool.claimRewards() n-1 more times to drain contract.
• Attack
  1. When he calls ConcurRewardPool.claimRewards() for the first time, the pBTC reward tokens are transferred.
  2. You can see from the pBTC contract on line 871 that _callTokensToSend(from, from, recipient, amount, "", ""); is called inside the transfer() function.
  3. If you trace to the _callTokensToSend function definition to line 1147, you will notice that it calls IERC777Sender(implementer).tokensToSend(operator, from, to, amount, userData, operatorData); on line 1159.
  4. Since the malicious user already registered a bad tokensToSend() function, this function will be called thus draining majority of the pBTC rewards available on the ConcurRewardPool contract.

You can also find a walkthrough replicating a similar attack here.

Recommended Mitigation Steps

• Use a nonReentrant modifier
• set balances to 0 first before disbursing the rewards

[GalloDaSballo]

The warden has shown how using a specific reward token can lead to reentrnacy for the function claimRewads

Because the finding is contingent on a specific token that enables the exploit, I believe Medium Severity to be appropriate