Submitted by hyh
Reward token accounting update in deposit() and withdraw() happens after reward transfer. If reward token allows for the control of transfer call flow or can be upgraded to allow it in the future (i.e. have or can introduce the _beforetokentransfer, _afterTokenTransfer type of hooks; or, say, can be upgraded to ERC777), the current implementation makes it possible to drain all the reward token funds of the contract by directly reentering deposit() or withdraw() with tiny _amount.
Setting the severity to medium as this is conditional to transfer flow control assumption, but the impact is the full loss of contract reward token holdings.
Both withdraw() and deposit() have the issue, performing late accounting update and not controlling for reentrancy:
ConvexMasterChef.sol#L209-L221
ConvexMasterChef.sol#L239-L250
Consider adding a direct reentrancy control, e.g. nonReentrant modifier:
https://docs.openzeppelin.com/contracts/2.x/api/utils#ReentrancyGuard
Also, consider finishing all internal state updates prior to external calls:
https://consensys.github.io/smart-contract-best-practices/attacks/reentrancy/#pitfalls-in-reentrancy-solutions
0xMaharishi (Aura Finance) confirmed and commented:
0xMaharishi (Aura Finance) resolved:
