c4-bot-2 commented 7 months ago

Lines of code

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

Vulnerability details

Impact

The receive() function in the ManagedWallet contract is used to approve address changes. By sending greater than or equal to 0.05 ether to the contract, however after the confirmation, the confirmationWallet can DOS the proposed Wallet forever by continuously sending 0.05 ether every 29 days. This prevents finalization as it continues shifting the activeTimelock by 30 days on every call

Proof of Concept

In the receive function, the contract increases the timelock every time the confirmation wallet sends it over 0.05 ether.

    // The confirmation wallet confirms or rejects wallet proposals by sending a specific amount of ETH to this contract
    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.
        //@audit Medium  confirmation wallet can DOS wallet change
        if ( msg.value >= .05 ether )
            activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock

Hence the activeTimelock never ends.

Tools Used

Manual Review

Recommended Mitigation Steps

Create a boolean value which returns true everytime the confirmation wallet has confirmed. it should return false after change or after rejection and it should be require in receive()

Assessed type

DoS

c4-judge commented 7 months ago

Picodes marked the issue as primary issue

c4-sponsor commented 6 months ago

othernet-global (sponsor) disputed

othernet-global commented 6 months ago

Yes, the confirmationWallet by design has the ability to prevent changes to the managed wallet.

c4-judge commented 6 months ago

Picodes changed the severity to QA (Quality Assurance)

Picodes commented 6 months ago

Downgrading to QA as this is among the confirmationWallet's powers