Ambire Wallet - Invitational - adriro's results

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L92

Vulnerability details

Fallback handlers can trick users into calling functions of the AmbireAccount contract

Selector clashing can be used to trick users into calling base functions of the wallet.

Impact

Fallback handlers provide extensibility to the Ambire wallet. The main idea here is that functions not present in the wallet implementation are delegated to the fallback handler by using the fallback() function.

Function dispatch in Solidity is done using function selectors. Selectors are represented by the first 4 bytes of the keccak hash of the function signature (name + argument types). It is possible (and not computationally difficult) to find different functions that have the same selector.

This means that a malicious actor can craft a fallback handler with a function signature carefully selected to match one of the functions present in the base AmbireAccount contract, and with an innocent looking implementation. While the fallback implementation may seem harmless, this function when called will actually trigger the function in the base AmbireAccount contract. This can be used, for example, to hide a call to setAddrPrivilege() which could be used to grant control of the wallet to the malicious actor.

This is similar to the exploit reported on proxies in this article, which caused the proposal of the transparent proxy pattern.

As further reference, another similar issue can be found in the DebtDAO contest that could lead to unnoticed calls due to selector clashing (disclaimer: the linked report is authored by me).

Recommendation

It is difficult to provide a recommendation based on the current design of contracts. Any whitelisting or validation around the selector won't work as the main entrypoint of the wallet is the AmbireAccount contract itself. The solution would need to be based on something similar to what was proposed for transparent proxies, which involves segmenting the calls to avoid clashing, but this could cripple the functionality and simplicity of the wallet.

Assessed type

Other

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L119-L123

Vulnerability details

Attacker can force the failure of transactions that use tryCatch

An attacker or malicious relayer can force the failure of transactions that rely on tryCatch() by carefully choosing the gas limit.

Impact

The tryCatch() function present in the AmbireAccount contract can be used to execute a call in the context of a wallet that is eventually allowed to fail, i.e. the operation doesn't revert if the call fails.

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L119-L123

119: 	function tryCatch(address to, uint256 value, bytes calldata data) external payable {
120: 		require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
121: 		(bool success, bytes memory returnData) = to.call{ value: value, gas: gasleft() }(data);
122: 		if (!success) emit LogErr(to, value, data, returnData);
123: 	}

EIP-150 introduces the "rule of 1/64th" in which 1/64th of the available is reserved in the calling context and the rest of it is forward to the external call. This means that, potentially, the called function can run out of gas, while the calling context may have some gas to eventually continue and finish execution successfully.

A malicious relayer, or a malicious actor that front-runs the transaction, can carefully choose the gas limit to make the call to tryCatch() fail due out of gas, while still saving some gas in the main context to continue execution. Even if the underlying call in tryCatch() would succeed, an attacker can force its failure, while the main call to the wallet is successfully executed.

Proof of Concept

The following test reproduces the attack. The user creates a transaction to execute a call using tryCatch() to a function of the TestTryCatch contract, which simulates some operations that consume gas. The attacker then executes the bundle by carefully choosing the gas limit (450,000 units of gas in this case) so that the call to TestTryCatch fails due to out of gas, but the main call to execute() in the wallet (here simplified by using executeBySender() to avoid signatures) gets correctly executed.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

contract TestTryCatch {
    uint256[20] public foo;

    function test() external {
        // simulate expensive operation
        for (uint256 index = 0; index < 20; index++) {
            foo[index] = index + 1;
        }
    }
}

function test_AmbireAccount_ForceFailTryCatch() public {
    address user = makeAddr("user");

    address[] memory addrs = new address[](1);
    addrs[0] = user;
    AmbireAccount account = new AmbireAccount(addrs);

    TestTryCatch testTryCatch = new TestTryCatch();

    AmbireAccount.Transaction[] memory txns = new AmbireAccount.Transaction[](1);
    txns[0].to = address(account);
    txns[0].value = 0;
    txns[0].data = abi.encodeWithSelector(
        AmbireAccount.tryCatch.selector,
        address(testTryCatch),
        uint256(0),
        abi.encodeWithSelector(TestTryCatch.test.selector)
    );

    // This should actually be a call to "execute", we simplify the case using "executeBySender"
    // to avoid the complexity of providing a signature. Core issue remains the same.
    vm.expectEmit(true, false, false, false);
    emit LogErr(address(testTryCatch), 0, "", "");
    vm.prank(user);
    account.executeBySender{gas: 450_000}(txns);

    // assert call to TestTryCatch failed
    assertEq(testTryCatch.foo(0), 0);
}

Recommendation

The context that does the call in tryCatch() can check the remaining gas after the call to determine if the remaining amount is greater than 1/64 of the available gas before the external call.

  function tryCatch(address to, uint256 value, bytes calldata data) external payable {
      require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
+     uint256 gasBefore = gasleft();
      (bool success, bytes memory returnData) = to.call{ value: value, gas: gasleft() }(data);
+     require(gasleft() > gasBefore/64);
      if (!success) emit LogErr(to, value, data, returnData);
  }

Assessed type

Other

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L144

Vulnerability details

Recovery transaction can be replayed after a cancellation

The recovery transaction can be replayed after a cancellation of the recovery procedure, reinstating the recovery mechanism.

Impact

The Ambire wallet provides a recovery mechanism in which a privilege can recover access to the wallet if they lose their keys. The process contains three parts, all of them considered in the execute() function:

1. A transaction including a signature with SIGMODE_RECOVER mode enqueues the transaction to be executed after the defined timelock. This action should include a signature by one of the defined recovery keys to be valid.
2. This can be followed by two paths, the cancellation of the process or the execution of the recovery:
   • If the timelock passes, then anyone can complete the execution of the originally submitted bundle.
   • A signed cancellation can be submitted to abort the recovery process, which clears the state of scheduledRecoveries.

Since nonces are only incremented when the bundle is executed, the call that triggers the recovery procedure can be replayed as long as the nonce stays the same.

This means that the recovery process can be re-initiated after a cancellation is issued by replaying the original call that initiated the procedure.

Note that this also works for cancellations. If the submitted recovery bundle is the same, then a cancellation can be replayed if the recovery process is initiated again while under the same nonce value.

Proof of Concept

1. Recovery process is initiated using a transaction with SIGMODE_RECOVER signature mode.
2. Procedure is canceled by executing a signed call with SIGMODE_CANCEL signature mode.
3. Recovery can be re-initiated by replaying the transaction from step 1.

Recommendation

Increment the nonce during a cancellation. This will step the nonce preventing any previous signature from being replayed.

...
  if (isCancellation) {
    delete scheduledRecoveries[hash];
+   nonce = currentNonce + 1;
    emit LogRecoveryCancelled(hash, recoveryInfoHash, recoveryKey, block.timestamp);
  } else {
    scheduledRecoveries[hash] = block.timestamp + recoveryInfo.timelock;
    emit LogRecoveryScheduled(hash, recoveryInfoHash, recoveryKey, currentNonce, block.timestamp, txns);
  }
  return;
  ...

Assessed type

Other

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L2

Vulnerability details

Project may fail to be deployed to chains not compatible with Shanghai hardfork

Current settings may produce incompatible bytecode with some of the chains supported by the protocol.

Impact

The Ambire wallet supports and targets different chains, such as Ethereum, Polygon, Avalanche, BNB, Optimism, Arbitrum, etc. This information is available in their website.

All of the contracts in scope have the version pragma fixed to be compiled using Solidity 0.8.20. This new version of the compiler uses the new PUSH0 opcode introduced in the Shanghai hard fork, which is now the default EVM version in the compiler and the one being currently used to compile the project.

Here is an excerpt of the bytecode produced for the AmbireAccount contract in which we can see the presence of the PUSH0 opcode(full bytecode can be found in the file artifacts/contracts/AmbireAccount.sol/AmbireAccount.json):

This means that the produced bytecode for the different contracts won't be compatible with the chains that don't yet support the Shanghai hard fork.

This could also become a problem if different versions of Solidity are used to compile contracts for different chains. The differences in bytecode between versions can impact the deterministic nature of contract addresses, potentially breaking counterfactuality.

Recommendation

Change the Solidity compiler version to 0.8.19 or define an evm version that is compatible across all of the intended chains to be supported by the protocol (see https://book.getfoundry.sh/reference/config/solidity-compiler?highlight=evm_vers#evm_version).

Assessed type

Other

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L58-L65

Vulnerability details

AmbireAccount implementation can be destroyed by privileges

The AmbireAccount implementation can be destroyed, resulting in the bricking of all associated wallets.

Impact

The AmbireAccount contract has a constructor that setups privileges, these are essentially addresses that have control over the wallet.

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L58-L65

58: 	constructor(address[] memory addrs) {
59: 		uint256 len = addrs.length;
60: 		for (uint256 i = 0; i < len; i++) {
61: 			// NOTE: privileges[] can be set to any arbitrary value, but for this we SSTORE directly through the proxy creator
62: 			privileges[addrs[i]] = bytes32(uint(1));
63: 			emit LogPrivilegeChanged(addrs[i], bytes32(uint(1)));
64: 		}
65: 	}

Normally this constructor is not really used, as wallets are deployed using proxies. The proxy constructor is the actual piece of code that setups the privileges storage to grant initial permission to the owner of the wallet.

However these proxies need to rely on a reference implementation of the AmbireAccount contract. A single contract is deployed and its address is then injected into the proxy code.

The main issue is that privileges defined in the reference implementation have control over that instance, and could eventually force a destruction of the contract using a fallback handler with a selfdestruct instruction (see PoC for a detailed explanation). This destruction of the implementation would render all wallets non-functional, as the proxies won't have any underlying logic code. Consequently, wallets would become inaccessible, resulting in potential loss of funds.

It is not clear the purpose of this constructor in the AmbireAccount contract. It may be present to facilitate testing. This issue can be triggered by a malicious deployer (or any of the defined privileges) or by simply setting up a wrong privilege accidentally. Nevertheless, its presence imposes a big and unneeded security risk, as the destruction of the reference implementation can render all wallets useless and inaccessible.

Proof of Concept

The following test reproduces the described issue. A deployer account deploys the implementation of the AmbireAccount contract that is later used by the user account to create a proxy (AccountProxy contract) over the implementation. The deployer then forces the destruction of the reference implementation using a fallback handler (Destroyer contract). The user's wallet is now inaccessible as there is no code behind the proxy.

The majority of the test is implemented in the setUp() function in order to properly test the destruction of the contract (in Foundry contracts are deleted when the test is finalized).

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

contract Destroyer {
    function destruct() external {
        selfdestruct(payable(address(0)));
    }
}

contract AccountProxy is ERC1967Proxy {
    // Simulate privileges storage
    mapping(address => bytes32) public privileges;

    constructor(address[] memory addrs, address _logic) ERC1967Proxy(_logic, "") {
		uint256 len = addrs.length;
		for (uint256 i = 0; i < len; i++) {
			// NOTE: privileges[] can be set to any arbitrary value, but for this we SSTORE directly through the proxy creator
			privileges[addrs[i]] = bytes32(uint(1));
		}
	}
}

contract AuditDestructTest is Test {
    AmbireAccount implementation;
    AmbireAccount wallet;

    function setUp() public {
        // Master account implementation can be destroyed by any of the configured privileges
        address deployer = makeAddr("deployer");
        address user = makeAddr("user");

        // Lets say deployer creates reference implementation
        address[] memory addrsImpl = new address[](1);
        addrsImpl[0] = deployer;
        implementation = new AmbireAccount(addrsImpl);

        // User deploys wallet
        address[] memory addrsWallet = new address[](1);
        addrsWallet[0] = user;
        wallet = AmbireAccount(payable(
            new AccountProxy(addrsWallet, address(implementation))
        ));

        // Test the wallet is working ok
        assertTrue(wallet.supportsInterface(0x4e2312e0));

        // Now privilege sets fallback
        Destroyer destroyer = new Destroyer();
        AmbireAccount.Transaction[] memory txns = new AmbireAccount.Transaction[](1);
        txns[0].to = address(implementation);
        txns[0].value = 0;
        txns[0].data = abi.encodeWithSelector(
            AmbireAccount.setAddrPrivilege.selector,
            address(0x6969),
            bytes32(uint256(uint160(address(destroyer))))
        );
        vm.prank(deployer);
        implementation.executeBySender(txns);

        // and destroys master implementation
        Destroyer(address(implementation)).destruct();
    }

    function test_AmbireAccount_DestroyImplementation() public {
        // Assert implementation has been destroyed
        assertEq(address(implementation).code.length, 0);

        // Now every wallet (proxy) that points to this master implementation will be bricked
        wallet.supportsInterface(0x4e2312e0);
    }
}

Recommendation

Remove the constructor from the AmbireAccount contract.

Assessed type

Other

Report

• Low Issues (2)
• Non Critical Issues (4)

Low Issues

<a name="L-1"></a>[L-1] AmbireAccountFactory can deploy backdoored code

The AmbireAccountFactory deploys new wallets by creating a contract using an arbitrary code given as a parameter. This can be used in a MITM attack to backdoor the wallet and install another privilege that would give the attacker control over the wallet.

Although this action may go unnoticed it is important to note that backdooring the code would change the address and could be early detected, hence the low severity.

<a name="L-2"></a>[L-2] splitSignature() does not validate length of signature

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/libs/SignatureValidator.sol#L29-L36

The splitSignature() does not validate that the length of the signature is greater than 0 and may end up causing an underflow in the unchecked block.

Non Critical Issues

<a name="NC-1"></a>[NC-1] Import declarations should import specific symbols

Prefer import declarations that specify the symbol(s) using the form import {SYMBOL} from "SomeContract.sol" rather than importing the whole file

Instances (3):

File: src/AmbireAccount.sol

4: import './libs/SignatureValidator.sol';

File: src/AmbireAccountFactory.sol

4: import './AmbireAccount.sol';

File: src/libs/SignatureValidator.sol

4: import './Bytes.sol';

<a name="NC-2"></a>[NC-2] Use named parameters for mapping type declarations

Consider using named parameters in mappings (e.g. mapping(address account => uint256 balance)) to improve readability. This feature is present since Solidity 0.8.18

Instances (2):

File: src/AmbireAccount.sol

13: 	mapping(address => bytes32) public privileges;

15: 	mapping(bytes32 => uint) public scheduledRecoveries;

<a name="NC-3"></a>[NC-3] Use uint256 instead of the uint alias

Prefer using the uint256 type definition over its uint alias.

Instances (1):

File: src/AmbireAccount.sol

15: 	mapping(bytes32 => uint) public scheduledRecoveries;

<a name="NC-4"></a>[NC-4] Unneeded explicit return

The explicit return can be omitted as the function is using named return data.

• https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/libs/Bytes.sol#L32