Confirmation wallet can bypass 30-day timelock completely or postpone confirmed wallet change infinitely

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L65-L66

Vulnerability details

Impact

Confirmation wallet can bypass 30-day timelock completely or postpone approved wallet change infinitely

Proof of Concept

In ManagedWallet.sol, confirmation wallet can confirm a proposal to change wallet addresses by sending greater than 0.05 ether to the contract.

However, the current implementation is flawed and allows two unsafe behaviors against intended design: (1) 30-day timelock bypass: There should be 30-day timelock after a proposal. But this can be bypassed by the current confirmation wallet send greater than 0.05 ether to the contract at the very beginning ( e.g. after contract deployment)

This way activeTimelock will be set to a known timestamp well before a proposal is made. Then when the mainwallet makes a proposal through proposeWallets(). The new proposedMainWallet can call changeWallets() instantly due to block.timestamp already passed a historic activeTimelock.

    receive() external payable
        {
        require( msg.sender == confirmationWallet, "Invalid sender" );

        // Confirm if .05 or more ether is sent and otherwise reject.
        // Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
        if ( msg.value >= .05 ether )
|>          activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L65-L66)

    function changeWallets() external
        {
        // proposedMainWallet calls the function - to make sure it is a valid address.
        require( msg.sender == proposedMainWallet, "Invalid sender" );
                //@audit : this require will pass without 30day timelock due to activeTimelock is a historic timestamp
|>      require( block.timestamp >= activeTimelock, "Timelock not yet completed" );

(https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L77) (2) infinite postpone of a confirmed proposal: Confirmation wallet can also postpone a confirmed proposal infinitely by sending 0.05 ether to the contract before the first 30-day timelock is passed, which effectively reset activeTimelock to a new future timestamp. 30-day timelock can be extended to whenever.

Tools Used

Manual

Recommended Mitigation Steps

In receive()- if ( msg.value >= .05 ether ) body, add checks to ensure proposedMainWallet != address(0) && activeTimelock == type(uint256).max

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #637

[c4-judge]

Picodes marked the issue as satisfactory