c4-bot-5 commented 8 months ago

Lines of code

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

Vulnerability details

Impact

Contract ManagedWallet.sol states that:

// A smart contract which provides two wallet addresses (a main and confirmation wallet) which can be changed using the following mechanism:
// 1. Main wallet can propose a new main wallet and confirmation wallet.
// 2. Confirmation wallet confirms or rejects.
// 3. There is a timelock of 30 days before the proposed mainWallet can confirm the change.

However, there is no check to stop confirmationWallet from approving an already approved proposal. This needs to be avoided as an accidental action because re-approving causes the activeTimelock to be pushed further into the future i.e. greater than 30 days.

The expected order of events by the protocol is:

proposeWallets() is called by mainWallet.
To confirm the proposal, confirmationWallet sends at least 0.05 ether and causes the receive() function to trigger. This sets the activeTimelock to block.timestamp + TIMELOCK_DURATION i.e. 30 days into the future.
proposedMainWallet calls changeWallets() after 30 days and the new wallet addresses are set.

Now consider the following flow:

proposeWallets() is called by mainWallet.
On t+0 day, to confirm the proposal, confirmationWallet sends at least 0.05 ether and causes the receive() function to trigger. This sets the activeTimelock to block.timestamp + TIMELOCK_DURATION i.e. t+30 days.
On t+15 days, confirmationWallet inadvertently sends 0.05 ether again which causes activeTimelock to move to t+45 days, unnecessarily delaying the change of wallet addresses.

Hence, I recommend that an already approved proposal be not allowed to be approved again.
Note that even if we consider that the protocol wants to give the freedom of pushing activeTimelock again & again into the future via multiple-approvals, it is much better to do so in the following manner (already supported by the current implementation):

a. Give 1st approval.
b. After X days (X is any value less than 30), if confirmationWallet wishes to push this further into the future, then:
- Reject the proposal by sending 0 ether to the contract
- Give 2nd approval

This is better because in the current design if an already approved proposal's activeTimelock is delayed inadvertently by confirmationWallet through re-approval, then there is no way to reset it to the previous value.

Proof of Concept

Add the following inside src/root_tests/ManagedWallet.t.sol and run with COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url https://rpc.sepolia.org/ --mt test_MultipleApprovalsCauseIncrementalDelays:

    function test_MultipleApprovalsCauseIncrementalDelays() public {
        // Set up the initial state with main and confirmation wallets
        address initialMainWallet = alice;
        address initialConfirmationWallet = address(0x2222);
        ManagedWallet managedWallet = new ManagedWallet(initialMainWallet, initialConfirmationWallet);

        // Set up the proposed main and confirmation wallets
        address newMainWallet = address(0x3333);
        address newConfirmationWallet = address(0x4444);

        // Prank as the initial main wallet to propose the new wallets
        vm.startPrank(initialMainWallet);
        managedWallet.proposeWallets(newMainWallet, newConfirmationWallet);
        vm.stopPrank();

        // Prank as the current confirmation wallet and send ether to confirm the proposal
        uint256 sentValue = 0.05 ether;
        vm.prank(initialConfirmationWallet);
        vm.deal(initialConfirmationWallet, sentValue);
        (bool success,) = address(managedWallet).call{value: sentValue}("");
        assertTrue(success, "Confirmation of wallet proposal failed");

        // Warp the blockchain time to +15 days
        uint256 startTime = block.timestamp;
        vm.warp(startTime + 15 days);

        // confirm the proposal once more
        vm.prank(initialConfirmationWallet);
        vm.deal(initialConfirmationWallet, sentValue);
        (success,) = address(managedWallet).call{value: sentValue}("");
        assertTrue(success, "Confirmation of wallet proposal failed");

        // Warp the blockchain time to 30 days from the 1st approval
        vm.warp(startTime + 30 days);

        // Expect revert 
        vm.expectRevert("Timelock not yet completed");
        vm.prank(newMainWallet);
        managedWallet.changeWallets();

        // Warp the blockchain time to another 15 days i.e. 30 days from the 2nd approval
        vm.warp(startTime + 45 days);

        // Should pass now
        vm.prank(newMainWallet);
        managedWallet.changeWallets();

        // Check that the mainWallet and confirmationWallet state variables are updated
        assertEq(managedWallet.mainWallet(), newMainWallet, "mainWallet was not updated correctly");
        assertEq(managedWallet.confirmationWallet(), newConfirmationWallet, "confirmationWallet was not updated correctly");

        // Check that the proposed wallets and activeTimelock have been reset
        assertEq(managedWallet.proposedMainWallet(), address(0), "proposedMainWallet was not reset");
        assertEq(managedWallet.proposedConfirmationWallet(), address(0), "proposedConfirmationWallet was not reset");
        assertEq(managedWallet.activeTimelock(), type(uint256).max, "activeTimelock was not reset to max uint256");
    }

Tools used

Foundry

Recommended Mitigation Steps

Inside receive() make sure we are not approving an already approved proposal by checking the current value of 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 )
+       {
+         if ( activeTimelock == type(uint256).max )  
            activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
+       }
        else
            activeTimelock = type(uint256).max; // effectively never
        }