Potential overflow on _lock() method
Unchecked accumulation in CSBondLock._lock() can theoretically overflow over repeated calls, even though access is restricted to onlyCSM.
Description
There is a potential for overflow in the _lock() function from the CSBondLock contract. The unchecked block in _lock() is problematic because it adds the new amount to any existing locked amount. Even if individual calls don't use extremely large values, repeated calls could potentially lead to an overflow over time.
function _lock(uint256 nodeOperatorId, uint256 amount) internal {...unchecked {if ($.bondLock[nodeOperatorId].retentionUntil > block.timestamp) {amount += $.bondLock[nodeOperatorId].amount;}_changeBondLock({nodeOperatorId: nodeOperatorId,amount: amount,retentionUntil: block.timestamp + $.bondLockRetentionPeriod});}}
Impact
The lockBondETH() function in CSModule (which calls _lock() in CSBondLock) is restricted to onlyCSM, meaning it can only be called by the CSM contract itself. This significantly reduces the risk of malicious exploitation. However, there is still theoretical risk even in unintentional circumstances.
Recommendation
Consider applying either of the following mitigations:
Add an explicit check for overflow:
if ($.bondLock[nodeOperatorId].retentionUntil > block.timestamp) {require(amount <= type(uint256).max - $.bondLock[nodeOperatorId].amount, "Lock amount too high");amount += $.bondLock[nodeOperatorId].amount;}
Or add an upper limit to the amount that can be locked, based on realistic maximum values for the protocol:
if ($.bondLock[nodeOperatorId].retentionUntil > block.timestamp) {require(amount <= MAX_LOCKABLE_AMOUNT, "Lock amount exceeds maximum");amount += $.bondLock[nodeOperatorId].amount;}
