The proposedMainWallet and proposedConfirmationWallet are not reset after confirmationWallet rejects wallet proposals

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/ManagedWallet.sol#L67-L68

Vulnerability details

Details

    function proposeWallets( address _proposedMainWallet, address _proposedConfirmationWallet ) external
        {
        require( msg.sender == mainWallet, "Only the current mainWallet can propose changes" );
        require( _proposedMainWallet != address(0), "_proposedMainWallet cannot be the zero address" );
        require( _proposedConfirmationWallet != address(0), "_proposedConfirmationWallet cannot be the zero address" );

        // Make sure we're not overwriting a previous proposal (as only the confirmationWallet can reject proposals)
@>      require( proposedMainWallet == address(0), "Cannot overwrite non-zero proposed mainWallet." );

@>      proposedMainWallet = _proposedMainWallet;
        proposedConfirmationWallet = _proposedConfirmationWallet;

        emit WalletProposal(proposedMainWallet, proposedConfirmationWallet);
        }

When calling proposeWallets, it will check if proposedMainWallet is address(0) to make sure we're not overwriting a previous proposal.

    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
        else
@>          activeTimelock = type(uint256).max; // effectively never
        }

    function changeWallets() external
        {
        // proposedMainWallet calls the function - to make sure it is a valid address.
        require( msg.sender == proposedMainWallet, "Invalid sender" );
        require( block.timestamp >= activeTimelock, "Timelock not yet completed" );

        // Set the wallets
        mainWallet = proposedMainWallet;
        confirmationWallet = proposedConfirmationWallet;

        emit WalletChange(mainWallet, confirmationWallet);

        // Reset
        activeTimelock = type(uint256).max;
@>      proposedMainWallet = address(0);
        proposedConfirmationWallet = address(0);
        }

However, after rejecting wallet proposals, the proposedMainWallet and proposedConfirmationWallet are not reset, resulting in the main wallet being unable to call proposeWallets function. The proposedMainWallet and proposedConfirmationWallet will only be reset after the changeWallets function is called. However, calling this function requires confirmation of confirmation wallet, which is obviously contrary to what was mentioned above.

Impact

Once the confirmation wallet rejects wallet proposals, the main wallet can no longer call the proposeWallets function to change mainWallet and confirmationWallet to new ones. This will result in the normal functionality of the contract not functioning properly.

Proof of Concept

Here is a possible attack scenario:

1. The main wallet calls the proposeWallets function, but the _proposedMainWallet and _proposedConfirmationWallet are incorrectly filled.
2. The confirmation wallet rejects wallet proposals, the changeWallets function can never be executed, so proposedMainWallet will never be set to 0.
3. Because the main wallet previously called the proposeWallets function with incorrect parameters, the main wallet now wants to call proposeWallets again with the correct parameters.
4. Since proposedMainWallet is not equal to address(0), apparently the main wallet cannot successfully call the proposeWallets function.
5. mainWallet and confirmationWallet will never be changed.

Tools Used

Visual Studio Code, Manual Code Review

Recommended Mitigation Steps

After rejecting wallet proposals, set proposedMainWallet and proposedConfirmationWallet to address(0).

    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
        else
++          {
            activeTimelock = type(uint256).max; // effectively never
++          proposedMainWallet = address(0);
++          proposedConfirmationWallet = address(0);
++          }
        }

Assessed type

Error

[c4-judge]

Picodes marked the issue as duplicate of #838

[c4-judge]

Picodes marked the issue as satisfactory