`updateDistribution()` can unexpectedly revert

[code423n4]

Handle

sirhashalot

Vulnerability details

Impact

The updateDistribution() can revert unexpectedly, which results in the _pointsPerUnit state variable not getting updated. Even more impactful is if the value of distributableXDEFI ever reaches a peak and never returns to this number, it will be impossible to call updateDistribution() without a revert.

The cause is that the safe casting _toUint256Safe(_updateXDEFIBalance()) reverts if _updateXDEFIBalance() is negative, which is possible. The return value of _updateXDEFIBalance() is an int, not a uint, and the value returned is _toInt256Safe(currentDistributableXDEFI) - _toInt256Safe(previousDistributableXDEFI) which can be negative if previousDistributableXDEFI > currentDistributableXDEFI. If this happens, the amount of XDEFI must be increased before the _pointsPerUnit state variable can be updated.

Proof of Concept

Line 147 of XDEFIDistribution.sol could revert if _updateXDEFIBalance() returns a negative int256 value https://github.com/XDeFi-tech/xdefi-distribution/blob/3856a42df295183b40c6eee89307308f196612fe/contracts/XDEFIDistribution.sol#L147

Recommended Mitigation Steps

The easiest fix is to remove the _toUint256Safe() casting on line 147 so that the _pointsPerUnit variable can decrease if newXDEFI is negative. If this is done, the newXDEFI variable should also become int256 instead of uint256. If the code is left as is, it should be documented that updateDistribution() can revert in certain circumstances and could become unusable depending on the distributableXDEFI value.

[deluca-mike]

While the result of _updateXDEFIBalance() is an int256, I can't see how it can ever be negative when called by updateDistribution(), since distributableXDEFI = IERC20(XDEFI).balanceOf(address(this)) - totalDepositedXDEFI and:

• locks increase IERC20(XDEFI).balanceOf(address(this)) and totalDepositedXDEFI by the same amount, and
• unlocks decrease IERC20(XDEFI).balanceOf(address(this)) and totalDepositedXDEFI by the same amount, and call _updateXDEFIBalance once they are done, which decreases distributableXDEFI

So, any call to updateDistribution() will always result in the newly computed distributableXDEFI to be greater than the current distributableXDEFI, and thus the return of _updateXDEFIBalance() would be greater or equal to zero.

If there was some proof of concept using the hardhat test file to demonstrate the issue, it would be easier, but without it, I cannot reproduce it, and it does not seem like this is a valid issue.

[Ivshti]

@deluca-mike this is an invalid finding because the condition seems impossible (unless the user proves it), but is there a reason why an int is used there rather than an uint?