Impossible to change managed wallets with `proposeWallets` after first rejection

mediumCode4rena

Lines of code

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

Vulnerability details

Issue Description

The receive function in ManagedWallet fails to reset the proposedMainWallet address to the zero address (address(0)) when the confirmation wallet rejects the wallet change:

solidity
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
    //@audit doesn't reset proposedMainWallet to address(0)
}

This omission renders it impossible to submit new proposals for changing wallets in the future. After the confirmation wallet rejects for the first time, the proposedMainWallet remains different from address(0), causing the proposeWallets function to revert due to the check on the proposedMainWallet address:

solidity
function proposeWallets(
    address _proposedMainWallet,
    address _proposedConfirmationWallet
) external {
    ...

    //@audit revert if proposedMainWallet != address(0)
    // 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);
}

Impact

After the first rejection of wallet changes, the proposeWallets function will consistently revert, making it impossible to change the main and confirmation wallets indefinitely.

Proof of Concept

The scenario for this issue to occurs is straightforward :

mainWallet make a request to proposeWallets to change the main and confirmation wallets which sets proposedMainWallet != address(0).
confirmationWallet doesn't like the change so he calls receive function with insufficient amount of ETH to refuse the change. After receive call, activeTimelock is set to uint256.max but proposedMainWallet is still different from address(0).
mainWallet tries to call proposeWallets again by it will always revert because proposedMainWallet != address(0).
Both main and confirmation wallets can never be changed again.

Tools Used

Manual review, VS Code

Recommended Mitigation

In the receive function, reset the proposedMainWallet variable to address(0) after the confirmation process has been rejected (similar to the reset done in changeWallets()).

solidity
if (msg.value >= .05 ether)
    activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
else {
    //@audit Reset
    activeTimelock = type(uint256).max;
    proposedMainWallet = address(0);
    proposedConfirmationWallet = address(0);
}