Potential re-entrancy

[code423n4]

Lines of code

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

Vulnerability details

Impact

A malicious user can potentially empty the rewards pool in the ConcurRewardPool.sol contract when withdrawing their rewards since their balance is only updated after making the external call to the token address and transferring the tokens. I have analyzed different attack scenarios and the most likely seems to be when the user passes in an ERC-777 token and leverages the callbacks to perform re-entrancy.

Another scenario lies in if a user somehow manages to pass in a malicious token with a modified safeTransfer function.

Proof of Concept

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConcurRewardPool.sol#L34-L38 Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Manual code review

Recommended Mitigation Steps

1. Update msg.sender's reward amount before making the external call to the token contract i.e move the update line from line 38 to 37.
2. Inherit the Reentrancy guard OpenZeppelin contract and ,ark the claimRewards() function as nonReentrant

[r2moon]

https://github.com/code-423n4/2022-02-concur-findings/issues/260

[GalloDaSballo]

Dup of #86