search

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

1 stars 0 forks source link

The Same address List of Owners with the same nonce will deploy different coinbase wallet #68

Open c4-bot-5 opened 6 months ago

c4-bot-5 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWalletFactory.sol#L28-L56

Vulnerability details

Impact

The same address list should give the same address based on the assumption that if the parameters passed are the same but the create account in the factory will deploy 2 different address if the same address are passed when the position of the address are switched with the same nonce. This error will break the already established fact that the same owners list and nonce should give the same address and revert without creating another account. But will not follow this assertion hence invalidating it. The impact of this issue is critical as it undermines the expected behavior of the system regarding account creation. Specifically, it disrupts the deterministic nature of account address generation, potentially leading to confusion, data inconsistencies, and security concerns. If left unaddressed, this issue could compromise the integrity and reliability of the system.

Proof of Concept

The vulnerability can be demonstrated through the following test scenario:

  1. Alice creates an owner list containing two addresses: Address 1 and Address 2, passing Address 1 before Address 2, and creates an account.
  2. Alice removes the list and passes the same addresses, but this time with Address 2 before Address 1, resulting in the creation of another account with a different address for Alice.

The provided test scenario confirms that the system behaves inconsistently when the order of addresses in the owner list is changed, leading to the creation of multiple accounts for the same owner.

function setUp() public {
    account = new CoinbaseSmartWallet();
    factory = new CoinbaseSmartWalletFactory(address(account));
    owners.push(abi.encode(address(1)));
    owners.push(abi.encode(address(2)));
}
  function test_createAccountDeploysToPredeterminedAddress() public {
        address p = factory.getAddress(owners, 0);
        CoinbaseSmartWallet a = factory.createAccount{value: 1e18}(owners, 0);
        assertEq(address(a), p);

        owners.pop();
  owners.pop();
  owners.push(abi.encode(address(2)));
  owners.push(abi.encode(address(1)));

 address pp = factory.getAddress(owners, 0);
 CoinbaseSmartWallet aa = factory.createAccount{value: 1e18}(owners,0);
 assertEq(address(aa), pp);

 assertEq(address(aa), address(a));

1 gives 0x761f8B0050e1C64663cdf67b163e139e74f2DDAd::isOwnerAddress;

2 gives0x00b70D9C493558d6F201966236B643EE44a43f30::isOwnerAddress;

Ran 1 test for test/SmartWallet/CoinbaseSmartWalletFactory.t.sol:CoinbaseSmartWalletFactoryTest
[FAIL. Reason: assertion failed] test_createAccountDeploysToPredeterminedAddress() (gas: 545531)
Logs:
  Error: a == b not satisfied [address]
        Left: 0x00b70D9C493558d6F201966236B643EE44a43f30
       Right: 0x761f8B0050e1C64663cdf67b163e139e74f2DDAd

Traces:
  [545531] CoinbaseSmartWalletFactoryTest::test_createAccountDeploysToPredeterminedAddress()
    ├─ [1979] CoinbaseSmartWalletFactory::getAddress([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77]) [staticcall]
    │   └─ ← 0x761f8B0050e1C64663cdf67b163e139e74f2DDAd
    ├─ [241054] CoinbaseSmartWalletFactory::createAccount{value: 1000000000000000000}([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])    
    │   ├─ [34337] → new <unknown>@0x761f8B0050e1C64663cdf67b163e139e74f2DDAd
    │   │   └─ ← 95 bytes of code
    │   ├─ [168692] 0x761f8B0050e1C64663cdf67b163e139e74f2DDAd::initialize([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002])
    │   │   ├─ [165886] CoinbaseSmartWallet::initialize([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002]) [delegatecall]
    │   │   │   ├─ emit AddOwner(index: 0, owner: 0x0000000000000000000000000000000000000000000000000000000000000001)
    │   │   │   ├─ emit AddOwner(index: 1, owner: 0x0000000000000000000000000000000000000000000000000000000000000002)
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   └─ ← 0x761f8B0050e1C64663cdf67b163e139e74f2DDAd
    ├─ [1979] CoinbaseSmartWalletFactory::getAddress([0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000001], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77]) [staticcall]
    │   └─ ← 0x00b70D9C493558d6F201966236B643EE44a43f30
    ├─ [238554] CoinbaseSmartWalletFactory::createAccount{value: 1000000000000000000}([0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000001], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])    
    │   ├─ [34337] → new <unknown>@0x00b70D9C493558d6F201966236B643EE44a43f30
    │   │   └─ ← 95 bytes of code
    │   ├─ [166192] 0x00b70D9C493558d6F201966236B643EE44a43f30::initialize([0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000001])
    │   │   ├─ [165886] CoinbaseSmartWallet::initialize([0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000001]) [delegatecall]
    │   │   │   ├─ emit AddOwner(index: 0, owner: 0x0000000000000000000000000000000000000000000000000000000000000002)
    │   │   │   ├─ emit AddOwner(index: 1, owner: 0x0000000000000000000000000000000000000000000000000000000000000001)
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   └─ ← 0x00b70D9C493558d6F201966236B643EE44a43f30
    ├─ emit log(val: "Error: a == b not satisfied [address]")
    ├─ emit log_named_address(key: "      Left", val: 0x00b70D9C493558d6F201966236B643EE44a43f30)
    ├─ emit log_named_address(key: "     Right", val: 0x761f8B0050e1C64663cdf67b163e139e74f2DDAd)
    ├─ [0] VM::store(VM: [0x7109709ECfa91a80626fF3989D68f67F5b1DD12D], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001)
    │   └─ ← ()
    └─ ← ()

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.07ms

Ran 1 test suite in 2.07ms: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/SmartWallet/CoinbaseSmartWalletFactory.t.sol:CoinbaseSmartWalletFactoryTest
[FAIL. Reason: assertion failed] test_createAccountDeploysToPredeterminedAddress() (gas: 545531)

when the list of owner was enlogated the system upholds this assertion

 function setUp() public {
        account = new CoinbaseSmartWallet();
        factory = new CoinbaseSmartWalletFactory(address(account));
        owners.push(abi.encode(address(1)));
        owners.push(abi.encode(address(2)));
         owners.push(abi.encode(address(3)));
         owners.push(abi.encode(address(4)));
    }
   function test_createAccountDeploysToPredeterminedAddress() public {
        address p = factory.getAddress(owners, 0);
        CoinbaseSmartWallet a = factory.createAccount{value: 1e18}(owners, 0);
        assertEq(address(a), p);

        owners.pop();
  owners.pop();
  owners.push(abi.encode(address(2)));
  owners.push(abi.encode(address(1)));
  owners.push(abi.encode(address(3)));
  owners.push(abi.encode(address(4)));

 address pp = factory.getAddress(owners, 0);
 CoinbaseSmartWallet aa = factory.createAccount{value: 1e18}(owners,0);
 assertEq(address(aa), pp);

 assertEq(address(aa), address(a));

Ran 1 test for test/SmartWallet/CoinbaseSmartWalletFactory.t.sol:CoinbaseSmartWalletFactoryTest
[FAIL. Reason: AlreadyOwner(0x0000000000000000000000000000000000000000000000000000000000000002)] test_createAccountDeploysToPredeterminedAddress() (gas: 801156)
Traces:
  [792756] CoinbaseSmartWalletFactoryTest::test_createAccountDeploysToPredeterminedAddress()
    ├─ [2755] CoinbaseSmartWalletFactory::getAddress([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000000000000000000000000000004], 0) [staticcall]
    │   └─ ← 0xcd6C85B8890C999EE10eF30e9ce0aC831A7B6E58
    ├─ [385457] CoinbaseSmartWalletFactory::createAccount{value: 1000000000000000000}([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000000000000000000000000000004], 0)
    │   ├─ [34337] → new <unknown>@0xcd6C85B8890C999EE10eF30e9ce0aC831A7B6E58
    │   │   └─ ← 95 bytes of code
    │   ├─ [311578] 0xcd6C85B8890C999EE10eF30e9ce0aC831A7B6E58::initialize([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000000000000000000000000000004])
    │   │   ├─ [308736] CoinbaseSmartWallet::initialize([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000000000000000000000000000004]) [delegatecall]
    │   │   │   ├─ emit AddOwner(index: 0, owner: 0x0000000000000000000000000000000000000000000000000000000000000001)
    │   │   │   ├─ emit AddOwner(index: 1, owner: 0x0000000000000000000000000000000000000000000000000000000000000002)
    │   │   │   ├─ emit AddOwner(index: 2, owner: 0x0000000000000000000000000000000000000000000000000000000000000003)
    │   │   │   ├─ emit AddOwner(index: 3, owner: 0x0000000000000000000000000000000000000000000000000000000000000004)
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   └─ ← 0xcd6C85B8890C999EE10eF30e9ce0aC831A7B6E58
    ├─ [3532] CoinbaseSmartWalletFactory::getAddress([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000000000000000000000000000004], 0) [staticcall]
    │   └─ ← 0x5f01e364aA7bca426802DD1ac2275371Dc84eaE9
    ├─ [245225] CoinbaseSmartWalletFactory::createAccount{value: 1000000000000000000}([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000000000000000000000000000004], 0)
    │   ├─ [34337] → new <unknown>@0x5f01e364aA7bca426802DD1ac2275371Dc84eaE9
    │   │   └─ ← 95 bytes of code
    │   ├─ [169896] 0x5f01e364aA7bca426802DD1ac2275371Dc84eaE9::initialize([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000000000000000000000000000004])
    │   │   ├─ [169507] CoinbaseSmartWallet::initialize([0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000000000000000000000000000004]) [delegatecall]
    │   │   │   ├─ emit AddOwner(index: 0, owner: 0x0000000000000000000000000000000000000000000000000000000000000001)
    │   │   │   ├─ emit AddOwner(index: 1, owner: 0x0000000000000000000000000000000000000000000000000000000000000002)
    │   │   │   └─ ← AlreadyOwner(0x0000000000000000000000000000000000000000000000000000000000000002)
    │   │   └─ ← AlreadyOwner(0x0000000000000000000000000000000000000000000000000000000000000002)
    │   └─ ← AlreadyOwner(0x0000000000000000000000000000000000000000000000000000000000000002)
    └─ ← AlreadyOwner(0x0000000000000000000000000000000000000000000000000000000000000002)

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.00ms

Ran 1 test suite in 5.00ms: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/SmartWallet/CoinbaseSmartWalletFactory.t.sol:CoinbaseSmartWalletFactoryTest
[FAIL. Reason: AlreadyOwner(0x0000000000000000000000000000000000000000000000000000000000000002)] test_createAccountDeploysToPredeterminedAddress() (gas: 801156)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

The vulnerability was identified through manual testing and analysis, utilizing custom test scripts and the Foundry testing framework.

Recommended Mitigation Steps

The protocol should consider reverting the create wallet function when the number of owners is 2 or should review this function to ensure that the same addresses will not deploy different account with the same nonce

Assessed type

Error

c4-pre-sort commented 6 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 6 months ago

raymondfam marked the issue as high quality report

raymondfam commented 6 months ago

This would indeed be an issue and extend beyond securing SCW deployment on various chains for the same address.

c4-pre-sort commented 6 months ago

raymondfam marked the issue as duplicate of #46

c4-pre-sort commented 6 months ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 6 months ago

raymondfam marked the issue as primary issue

wilsoncusack commented 6 months ago

This is working as intended. Users need to pass original owners in the same order to get the same address.

c4-sponsor commented 6 months ago

wilsoncusack marked the issue as disagree with severity

c4-sponsor commented 6 months ago

wilsoncusack (sponsor) acknowledged

3docSec commented 6 months ago

User mistake with no impact other than purely UX - having to recreate the wallet with a different input order.

c4-judge commented 6 months ago

3docSec changed the severity to QA (Quality Assurance)

c4-judge commented 6 months ago

3docSec marked the issue as grade-a

c4-judge commented 6 months ago

This previously downgraded issue has been upgraded by 3docSec

3docSec commented 6 months ago

(temporarily upgrading to to dedupe #114)

c4-judge commented 6 months ago

3docSec changed the severity to QA (Quality Assurance)