Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Any owner can fully control the smart wallet. #115

Open
c4-bot-6 opened this issue Mar 21, 2024 · 9 comments
Open

Any owner can fully control the smart wallet. #115

c4-bot-6 opened this issue Mar 21, 2024 · 9 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-18 grade-a Q-11 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_08_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-6
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/CoinbaseSmartWallet.sol#L189-L212

Vulnerability details

Impact

Every owner of the smart wallet can add or remove other owners to/from the smart wallet, and can also remove all owners from the smart wallet.

Proof of Concept

Owners can interact with the smart wallet with the UserOperation and also can interact directly using execute and executeBatch function of the CoinbaseSmartWallet.

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/CoinbaseSmartWallet.sol#L189-L212

    /// @notice Execute the given call from this account.
    ///
    /// @dev Can only be called by the Entrypoint or an owner of this account (including itself).
    ///
    /// @param target The target call address.
    /// @param value  The call value to user.
    /// @param data   The raw call data.
    function execute(address target, uint256 value, bytes calldata data) public payable virtual onlyEntryPointOrOwner {
        _call(target, value, data);
    }

    /// @notice Execute the given list of calls from this account.
    ///
    /// @dev Can only be called by the Entrypoint or an owner of this account (including itself).
    ///
    /// @param calls The list of `Call`s to execute.
    function executeBatch(Call[] calldata calls) public payable virtual onlyEntryPointOrOwner {
        for (uint256 i; i < calls.length;) {
            _call(calls[i].target, calls[i].value, calls[i].data);
            unchecked {
                ++i;
            }
        }
    }

_call(target, value, data) allows any owner can do any operation using CoinbaseSmartWallet.

So, malicious owner can call this _call function to add or remove the owners of the CoinbaseSmartWallet.

And all of the asset and operations of the CoinbaseSmartWallet can be managed by any ower.

So it have the same vulnerabilities of the traditional EOA.

Tools Used

Manual review

Recommended Mitigation Steps

Add the additional sercurity functions to prevent the traditional vulnerabilities.

And restrict the permission to add or remove the owners.

Assessed type

Other

@c4-bot-6 c4-bot-6 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 21, 2024
c4-bot-5 added a commit that referenced this issue Mar 21, 2024
@c4-bot-13 c4-bot-13 added the 🤖_08_group AI based duplicate group recommendation label Mar 21, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Mar 21, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #18

@raymondfam
Copy link

See #18.

@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #22

@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #181

@c4-judge
Copy link
Contributor

3docSec marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

3docSec marked the issue as duplicate of #18

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Mar 27, 2024
@c4-judge
Copy link
Contributor

3docSec changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

3docSec marked the issue as grade-a

@C4-Staff C4-Staff reopened this Apr 1, 2024
@C4-Staff C4-Staff added the Q-11 label Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-18 grade-a Q-11 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_08_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

6 participants