Staker.sol: withdrawAll() does not include incoming outstanding shifts to the user

[code423n4]

Handle

hickuphh3

Vulnerability details

Impact

It should be made clear to users that withdrawAll() does not include pending incoming shifts that will increase their stake balances. For example, if a user chooses to withdraw all his staked long tokens, then pending short tokens that are to be shifted are not accounted for.

While it would be possible to account for the pending shifts that are confirmed, but not settled, ie. 0 < userNextPrice_stakedSyntheticTokenShiftIndex[marketIndex][msg.sender] < batched_stakerNextTokenShiftIndex[marketIndex], it would add complexity to the withdrawAll() function. Furthermore, it is not possible to account for pending shifts that require the next price snapshot to be taken first, ie. userNextPrice_stakedSyntheticTokenShiftIndex[marketIndex][msg.sender] = batched_stakerNextTokenShiftIndex[marketIndex].

Recommended Mitigation Steps

Consider renaming withdrawAll() to something that takes into account this behaviour, like withdrawAllExceptPendingIncomingShifts().

[JasoonS]

Interesting point. The pending incoming shifts shouldn't stay pending for more than a price update (our current heartbeat with chainlink is between 30-60 seconds), so unlikely an issue.