`proposedMainWallet` was not set to `address(0)` when a wallet proposal is rejected

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

There is no way to propose a new main wallet and confirmation wallet if the previous wallet proposal is rejected.

Proof of Concept

The mainWallet of ManagedWallet can propose a new main wallet and confirmation wallet:

  // Make a request to change the main and confirmation wallets.
  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);
  }

The confirmationWallet of ManagedWallet can confirm the proposal by sending a minimum of 0.05 ether to ManagedWallet; otherwise, it is rejected:

  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
  }

However, proposedMainWallet was not set to address(0) when the proposal is rejected. There is no way to create a new proposal unless confirmationWallet confirm the old one. Copy below codes to ManagedWallet.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testProposeWalletsAfterPreviousProposalIsRejected

  function testProposeWalletsAfterPreviousProposalIsRejected() public {
    ManagedWallet managedWallet = new ManagedWallet(alice, address(this));
    address proposedMainWallet = address(0x3333);
    address proposedConfirmationWallet = address(0x4444);

    vm.startPrank(alice);
    managedWallet.proposeWallets(proposedMainWallet, proposedConfirmationWallet);
    vm.stopPrank();

    assertEq(managedWallet.proposedMainWallet(), proposedMainWallet, "proposedMainWallet not set correctly");
    assertEq(managedWallet.proposedConfirmationWallet(), proposedConfirmationWallet, "proposedConfirmationWallet not set correctly");
    //@audit-info reject the proposal
    (bool success, ) = address(managedWallet).call{value: 0}("");

    //@audit-info activeTimelock was changed to type(uint256).max
    uint256 expectedTimelock = type(uint256).max;
    assertEq(managedWallet.activeTimelock(), expectedTimelock, "activeTimelock should be type(uint256).max");
    assertTrue(success, "Transaction should be successful");
    //@audit-info proposedMainWallet & proposedConfirmationWallet were not reset to address(0)
    assertEq(managedWallet.proposedMainWallet(), proposedMainWallet, "proposedMainWallet was cleaned");
    assertEq(managedWallet.proposedConfirmationWallet(), proposedConfirmationWallet, "proposedConfirmationWallet was cleaned");
    //@audit-info It is impossible to create a new proposal
    vm.expectRevert("Cannot overwrite non-zero proposed mainWallet.");
    vm.prank(alice);
    managedWallet.proposeWallets(proposedMainWallet, proposedConfirmationWallet);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Set proposedMainWallet to address(0) when rejecting the wallet 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
+     proposedMainWallet = address(0);
+   }
  }

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as duplicate of #838

[c4-judge]

Picodes marked the issue as satisfactory