(15) Potentially missing overflow checks

[oldchili]

Should we check for overflow here - https://github.com/makerdao/lockstake/blob/cab89b5a5d737e9f4e399b323884131e37adb2d6/src/LockstakeEngine.sol#L400

and here - https://github.com/makerdao/lockstake/blob/cab89b5a5d737e9f4e399b323884131e37adb2d6/src/LockstakeClipper.sol#L393.

Didn't yet check deeply but I think at least semantically it might occur as ink is a uint256. Can consider using some generic function for the conversion like https://github.com/makerdao/dss-proxy-actions/blob/465c9b60688ebcab95b59fb97644f62aff837f10/src/DssProxyActions.sol#L15

[sunbreak1211]

I need to think about this deeper, but we start from the base that the auction kick can not have a lot higher than the max negative int256, as otherwise the vault can't be grabbed here: https://github.com/makerdao/dss/blob/master/src/dog.sol#L216C48-L216C75. So I'd think these two should be covered by this initial trigger. On the other hand it would be relying on an external contract (Dog), so maybe we should just add the requires for explicitness.

[oldchili]

Yes, might be better to cover it and not have to rely on the other contract.

[sunbreak1211]

I'm considering adding this to kick:

require(lot <= uint256(type(int256).max), "LockstakeClipper/lot-greater-maxint");

And then a require to onTakeLeftovers to not assume values coming from other contracts, but inside the Clipper it should ensure that the three slip calls won't revert due to overflow (yank flow has its own problems though that will be taken care in another issue).

[oldchili]

Sounds good.

[sunbreak1211]

Done: https://github.com/makerdao/lockstake/commit/f71497d602230c75d856bebb8bb5ee7cb71b1bd8