search

code-423n4 / 2024-03-coinbase-findings

1 stars 0 forks source link

wallet is missing a consensus mechanism for the wallet owners #134

Open c4-bot-10 opened 5 months ago

c4-bot-10 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L330 https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L196 https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L205

Vulnerability details

Impact

The current wallet implementation does not use any consensus mechanism allowing any owner to do the following dangerous things:

  1. One owner can remove all other including himself from owning the wallet. Leaving a wallet unusable.
  2. One owner can decide the movement of wallet funds. One malicious account can steal all the funds.
  3. One owner can decide on upgrade directions of the wallet contract.

Proof of Concept

The following snippet of code is used to check ownership permission on all important wallet functions:

    modifier onlyOwner() virtual {
        _checkOwner();
        _;
    }

    modifier onlyEntryPointOrOwner() virtual {
        if (msg.sender != entryPoint()) {
            _checkOwner();
        }

        _;
    }

    function _checkOwner() internal view virtual { // @-> all authentication come to this point where the only importance for ownership is a registred address
        if (isOwnerAddress(msg.sender) || (msg.sender == address(this))) {
            return;
        }

        revert Unauthorized();
    }

It is used in :

The _checkOwner will allow any registred address to pass the ownership check and in doing so grant all permission on a wallet.

Tools Used

Manual review

Recommended Mitigation Steps

I would suggest to use a consensus mechanism with a minimal amount of valid (buf from different owners) signatures before allowing authentication to important parts of the wallet contract. Something like is done in the Gnosis multisig wallet contract.

Assessed type

Access Control

c4-pre-sort commented 5 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #18

raymondfam commented 5 months ago

See #18.

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #22

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #181

c4-judge commented 5 months ago

3docSec marked the issue as satisfactory

c4-judge commented 5 months ago

3docSec changed the severity to QA (Quality Assurance)