Potential Re-entrancy Attack via ETH or ERC777 Token Transfer

[code423n4]

Lines of code

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

Vulnerability details

Impact

The CEI pattern is not being implemented properly in the claimRewards function of the ConcurRewardPool.sol.
https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol#L37

    function claimRewards(address[] calldata _tokens) external override {
        for (uint256 i = 0; i < _tokens.length; i++) {
            uint256 getting = reward[msg.sender][_tokens[i]];
            IERC20(_tokens[i]).safeTransfer(msg.sender, getting);
            reward[msg.sender][_tokens[i]] = 0;
        }
    }

On line 37, the safeTransfer method is called on the ERC20 token (note, safeTransfer does not protect against re-entrancy attacks) and THEN the reward value is set to 0. If this token is question is an ERC777 token with callbacks (this is possible as ERC777 is backward compatible and both Convex's VirtualBalanceRewardPool.sol and Concur's ConvexStakingWrapper.sol utilize IERC20 to interact with the token) then the attacker can utilize the callback to re-enter the function and drain the contract of rewards before the reward is set to 0.

This attack vector would also be present if future updates to the code enabled rewards to be automatically swapped to ETH and the same pattern was followed.

Tools Used

Manual review

Recommended Mitigation Steps

Follow the CEI pattern.

[GalloDaSballo]

Duplicate of #86