`StakingOperations` Token Transfer is updating the total supply before accruing rewards to users causing loss of rewards

[GalloDaSballo]

Impact

 function updatePool(ISwapPair _pid) public {
    PoolInfo storage pool = poolInfo[_pid];

    // check
    if (block.timestamp <= pool.lastRewardTime) return;

    // update
    uint tokenSupply = _pid.balanceOf(address(this));
    if (tokenSupply == 0 || totalAllocPoint == 0) {
      pool.lastRewardTime = block.timestamp;
      return;
    }
    uint multiplier = block.timestamp - pool.lastRewardTime;
    uint reward = (multiplier * rewardsPerSecond * pool.allocPoint) / totalAllocPoint;
    pool.accRewardPerShare += (reward * REWARD_DECIMALS) / tokenSupply;
    pool.lastRewardTime = block.timestamp;
  }

-> Transfer -> New Balance -> Check points (user uses old balance, but total supply is the new one) -> User lost rewards -> New total Supply and correct total debt

In fixing this be careful not to cause a division by zero (as the initial deposit would)

This can only happen on deposits (which dilute rewards), if the same mistake was done on withdrawals, then value could be stolen

Mitigation

Because the code is tightly coupled, I think accruing and then transferring the tokens could be sufficient

Alternatively the scalable fix is to track the previous balance in storage as you suggested

I think tracking in storage is the best fix long term (allows to re-use the contract separately without bugs)

[sambP]