c4-bot-8 commented 5 months ago

Lines of code

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

Vulnerability details

The receive() function is intended to accept Ether to confirm or reject wallet proposals. If 0.05 Ether or more is sent, it triggers a confirmation and starts a timelock. However, any Ether sent in excess of the 0.05 Ether threshold is not refunded to the sender.

Impact

The confirmation wallet may send more than the required 0.05 Ether for confirmation, either by mistake or due to a lack of precision. The excess Ether is not returned, potentially resulting in an unintended loss of funds.

Proof of Concept

See the below code for confirming or rejecting wallet proposals by sending 0.05 ETH. But the below receiver function is not implemented any refund mechanism for excess transfer.

File: src/ManagedWallet.sol

59:    receive() external payable
60:        {
61:        require( msg.sender == confirmationWallet, "Invalid sender" );
62:
63:                // Confirm if .05 or more ether is sent and otherwise reject.
64:                // Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
65:        if ( msg.value >= .05 ether )
66:                activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
67:        else
68:                        activeTimelock = type(uint256).max; // effectively never
69:        }

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

Steps to Reproduce

Invoke proposeWallets from the mainWallet with new wallet addresses.
Send an amount greater than 0.05 Ether to the contract from the confirmationWallet.
Notice that the surplus Ether is retained by the contract without a refund.

Tools Used

Manual Review

Recommended Mitigation Steps

Introduce a refund mechanism within the receive() function to return any surplus Ether above the 0.05 Ether confirmation requirement.

Suggested Fix

    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
+       if ( msg.value >= .05 ether ){
+               payable(msg.sender).call{value: msg.value - 0.05 ether}("");
+       }
        else
            activeTimelock = type(uint256).max; // effectively never
        }

Assessed type

ETH-Transfer

c4-judge commented 5 months ago

Picodes marked the issue as duplicate of #59

c4-judge commented 4 months ago

Picodes marked the issue as selected for report

Picodes commented 4 months ago

Out of scope per https://github.com/code-423n4/2024-01-salty-findings/issues/18