AbstractRewardMine - Re-entrancy attack during withdrawal

[code423n4]

Handle

ScopeLift

Vulnerability details

Impact

The internal _withdraw method does not follow the checks-effects-interactions pattern. A malicious token, or one that implemented transfer hooks, could re-enter the public calling function (such as withdraw()) before proper internal accounting was completed. Because the earned function looks up the _userWithdrawn mapping, which is not yet updated when the transfer occurs, it would be possible for a malicious contract to re-enter _withdraw repeatedly and drain the pool.

Proof of Concept

N/A

Tools Used

N/A

Recommended Mitigation Steps

The internal accounting should be done before the transfer occurs:

function _withdraw(address account, uint256 amountReward, address to) internal {
    _userWithdrawn[account] += amountReward;
    _globalWithdrawn += amountReward;f

   rewardToken.safeTransfer(to, amountReward);

    emit Withdraw(account, amountReward, to);
  }

[GalloDaSballo]

The warden identified a re-entrancy vulnerability that, given the right token would allow to drain the entirety of the contract.

Tokens with hooks (ERC777 and ERC677) would allow to exploit the contract and drain it in it's entirety.

This is a very serious vulnerability. However it can happen exclusively on a malicious or a token with hooks, as such (while I recommend the sponsor to mitigate by following recommendation by the warden), the attack can be completely prevented by using a token without hooks.

For that reason I'll rate the finding of medium severity (as it requires external conditions)