ManagedWallet: Rejecting a proposal to change the wallet addresses prohibits future proposals

[c4-bot-8]

Lines of code

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

Vulnerability details

A proposal to change the main and confirmation wallet addresses of a ManagedWallet can only be made if the current value of proposedMainWallet is address(0), i.e. no pending proposal exists (https://github.com/code-423n4/2024-01-salty/blob/main/src/ManagedWallet.sol#L49).

A proposal is accepted or rejected by sending Ether to the ManagedWallet from the current confirmation wallet. If the Ether amount sent is less than 0.05 Ether, the proposal is rejected. However, rejecting the proposal does not reset proposedMainWallet to address(0), so no future proposals can be made.

Impact

After a proposal to change the main and confirmation wallet addresses of a ManagedWallet has been rejected, no further proposals to change the wallet can be made, except if the proposal is accepted and then executed (which is still possible after rejecting it).

Proof of Concept

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

Proposing new addresses via proposeWallets() is only possible if proposedMainWallet == address(0) and this sets proposedMainWallet:

// Make a request to change the main and confirmation wallets.
function proposeWallets( address _proposedMainWallet, address _proposedConfirmationWallet ) external
    {
    [...]

    // 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;

    [...]

    }

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

Rejecting the proposal sets the activeTimelock to type(uint256).max so changeWallets() cannot be called to execute the change, but doesn't reset proposedMainWallet, prohibiting future proposals:

// The confirmation wallet confirms or rejects wallet proposals by sending a specific amount of ETH to this contract
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
    }

Recommended Mitigation Steps

Set proposedMainWallet to address(0) in the else branch in receive().

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as duplicate of #802

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #838

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid