c4-bot-8 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

Rejecting wallet proposals is useless, as the proposal variables are not reset.

This bricks the contract functionality, as it's not possible to propose different wallets.

Proof of Concept

The mainWallet has the ability to propose new wallets, which are tracked in storage.

Confirmation wallets have the ability to confirm or reject those proposed wallets.

The problem is that the corresponding storage variables proposedMainWallet, and proposedConfirmationWallet are not reset to 0 when they are 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 >= 0.05 ether) {
            activeTimelock = block.timestamp + TIMELOCK_DURATION;
        } // establish the timelock
        else {
            activeTimelock = type(uint256).max;
👉          // @audit proposals should be reset here
        } // effectively never
    }

ManagedWallet.sol#L58-L69

This bricks the contract functionality, as no new wallets can be proposed, since proposedMainWallet wasn't reset.

    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." );

ManagedWallet.sol#L48-L49

Recommended Mitigation Steps

Reset proposedMainWallet, and proposedConfirmationWallet when rejecting wallet proposals:

    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 >= 0.05 ether) {
            activeTimelock = block.timestamp + TIMELOCK_DURATION;
        } // establish the timelock
        else {
            activeTimelock = type(uint256).max;
+           proposedMainWallet = address(0);
+           proposedConfirmationWallet = address(0);
        } // effectively never
    }

Assessed type

Other

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #838

c4-judge commented 8 months ago

Picodes marked the issue as satisfactory

code-423n4 / 2024-01-salty-findings