Concur Finance contest - ShadowyNoobDev's results

Lines of code

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

Vulnerability details

[Shelter] Withdraw doesn’t check that claimant has withdrawn.

Impact

It allows a claimant to call Shelter.withdraw() multiple times to drain the pool.

Proof of Concept

Call Shelter.withdraw() multiple times

Recommended Mitigation Steps

Add a require check require(!claimed[_token][_to], "unable to claim again"); at the beginning of the withdraw() function to prevent a claimant from making multiple claims.

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

QA report

• ConvexStakingWrapper
  1. non critical, _getTotalSupply() function name is quite misleading because it doesn’t actually get you the total supply but rather, it gets you the total number of tokens staked in the ConvexStakingWrapper contract.
  2. low, it is possible to avoid rounding errors when calculating d_reward in the _calcRewardIntegral() function by calculating the 20%, storing it in a local variable and then update d_rewards by subtracting this 20% away instead of the current d_reward = (d_reward * 4) / 5;
  3. low, deposit() function has a bad comment in the natspec. Fix should be @param _amount amount to deposit.
  4. low, as confirmed with leekt on discord, this comment //no-op for cvx, crv rewards in the addRewards() function is deprecated and should be removed.

Gas optimisation

• ConvexStakingWrapper
  1. In the addRewards function, you should store IRewardStaking(convexBooster).poolInfo(_pid) into a locally scoped variable so it is cheaper to retrieve mainPool (line 94) and lptoken (line 98).
  2. In the addRewards function, you should use extraToken on line 131 instead of calling IRewardStaking(extraPool).rewardToken() to retrieve it again.
• MasterChef
  1. This require statement require(_token != address(0), "zero address"); in the add() function is IMO not required because only the owner can call this function.
  2. This require statement require(user.amount > 0, "MasterChef: nothing to withdraw"); in the withdraw() function is not needed because if user.amount is actually 0, the function will still work correctly as intended since the next require statement will ensure that the user cannot withdraw more than his balance.
  3. This if branch if (_amount > 0) in the withdraw() function is also not needed because user.amount is not modified when _amount is 0.