Users can accidentally lock their stakes forever

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L59 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/StakeManager.sol#L84

Vulnerability details

Impact

StakeManager accepts user deposits and stakes. When adding a new stake, it allows the arbitrary value of _unstakeDelaySec supposedly it is higher than previous info.unstakeDelaySec:

    function addStake(uint32 _unstakeDelaySec) public payable {
        DepositInfo storage info = deposits[msg.sender];
        require(_unstakeDelaySec > 0, "must specify unstake delay");
        require(_unstakeDelaySec >= info.unstakeDelaySec, "cannot decrease unstake time");
        ...
    }

If the users want to unlock their stakes, they can only withdraw when the unstakeDelaySec has passed:

    function unlockStake() external {
        ...
        uint64 withdrawTime = uint64(block.timestamp) + info.unstakeDelaySec;
        info.withdrawTime = withdrawTime;
        ...
    }

There is no upper boundary for the _unstakeDelaySec value. If users accidentally set a too big value for _unstakeDelaySec, their stake will be locked for life. This could happen if, for instance, a value of timestamp, not seconds are passed as a parameter. There should be a reasonable upper boundary for this value.

Recommended Mitigation Steps

The protocol should prevent users from making such mistakes. Introduce a reasonable upper limit for _unstakeDelaySec, e.g. 5 years.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Out of scope