bitsurfer - `setValidatorAddress` will not be usable in the long run due to `unstakings` array will eventually reach 300 array length

[sherlock-admin]

bitsurfer

medium

setValidatorAddress will not be usable in the long run due to unstakings array will eventually reach 300 array length

Summary

unstakings array will increase linearly, and since it's lack of clearing (and popped-out), it will eventually pass the 300 length, resulting setValidatorAddress will not be usable

Vulnerability Detail

When unstake, the unstakings will increase its length due to push on Line 535.

On the other hand, when clearing out (setting the unstaking amount to 0), via recoverUnstaking, transferUnstakedOut or redelegateUnstaked, the array is not popped out, only set the coolDownEnd to 0 if (us.amount == 0) us.coolDownEnd = 0;

This means, the design of unstakings will increase linearly, which potentially passed the 300 length size of unstakings, thus the setValidatorAddress will not usable because it only accept when the unstakings array length is less or equal than 300.

There might be some condition where validator need to change their address, due to this issue, the validator can't change the address.

File: OperationalStaking.sol
455:     /*
456:      * Undelegates some number of tokens from the provided validator
457:      */
458:     function unstake(uint128 validatorId, uint128 amount) external whenNotPaused {
459:         require(amount > 0, "Amount is 0");
460:         _unstake(validatorId, amount);
461:     }
462: 
463:     /*
464:      * Undelegates tokens from the provided validator
465:      */
466:     function _unstake(uint128 validatorId, uint128 amount) internal {
...
529:         // create unstaking instance
530:         uint128 coolDownEnd = uint128(v.disabledAtBlock != 0 ? v.disabledAtBlock : block.number);
531:         unchecked {
532:             coolDownEnd += (isValidator ? validatorCoolDown : delegatorCoolDown);
533:         }
534:         uint128 unstakeId = uint128(v.unstakings[msg.sender].length);
535:         v.unstakings[msg.sender].push(Unstaking(coolDownEnd, effectiveAmount));
536:         emit Unstaked(validatorId, msg.sender, effectiveAmount, unstakeId);
537:     }
...
689:     function setValidatorAddress(uint128 validatorId, address newAddress) external whenNotPaused {
...
700:         Unstaking[] storage oldUnstakings = v.unstakings[msg.sender];
701:         uint256 length = oldUnstakings.length;
702:         require(length <= 300, "Cannot transfer more than 300 unstakings");
...
711:     }

Impact

In the long run, setValidatorAddress will not work due to eventually it will reach unstakings array length limit of 300

Code Snippet

https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L535 https://github.com/sherlock-audit/2023-11-covalent/blob/main/cqt-staking/contracts/OperationalStaking.sol#L702

Tool used

Manual Review

Recommendation

Consider to clear out the unstakings if the amount and coolDownEnd are 0s, and switch the last index array data to the removed index, and poped the last item

Duplicate of #25

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: need a function to reduce the array lenght; medium(1)