Description:
StakingPool::unstake() reduces the units of the user pro-rata to the unstaked amount:
function unstake(uint256 amount) external {
...
// Get current units and calculate units to remove proportionally
// This maintains the exact proportional relationship between units and staked amount
uint128 currentUnits = distributionPool.getUnits(msg.sender);
uint128 unitsToRemove = uint128((amount * currentUnits) / userStakingInfo.stakedAmount);
// Update member units
distributionPool.decreaseMemberUnits(msg.sender, unitsToRemove);
// Update staking info
_stakingInfo[msg.sender].stakedAmount -= amount;
// If all tokens are unstaked, reset the staking info
if (_stakingInfo[msg.sender].stakedAmount == 0) {
delete _stakingInfo[msg.sender];
}
// Transfer staked tokens back to user
child.transfer(msg.sender, amount);
emit Unstaked(msg.sender, amount);
}The issue is that, as it rounds the units down, a staker is able to abuse it by unstaking many times, each time rounding it down, withdrawing all the stake, but keeping many units. Since 1000e18 stake is 1000 units, if they withdraw 1.9999e18, they remove only 1 unit, since 1.9999e18*1000/1000e18 = 1. By repeating this, they can unstake the full stake, and keep 50% staked.
Impact:
Stolen rewards.
Recommended Mitigation:
Round up. Also needs to make sure it doesn't underflow when reducing units.
0xSimao:
Fixed in PR #5.