Current design won't allow to update reference implementation without breaking counterfactuality

[code423n4]

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccountFactory.sol#L45-L47

Vulnerability details

Current design won't allow to update reference implementation without breaking counterfactuality

The current design of the Ambire wallet doesn't allow to update the reference implementation as doing so will break counterfactuality.

Impact

Ambire wallets are deployed by creating a thin proxy over a reference implementation of the AmbireAccount contract. The address of the implementation to which the proxy delegates calls is baked in the initcode of the proxy. This can be seen in the following snippet of IdentityProxyDeploy.js:

https://github.com/AmbireTech/adex-protocol-eth/blob/master/js/IdentityProxyDeploy.js#L43

return `0x${initialCode.toString('hex')}3d3981f3363d3d373d3d3d363d${evmPush(
  masterAddrBuf
).toString('hex')}5af43d82803e903d91602b57fd5bf3`

Counterfactuality depends on the initcode. Any change to the code will change the hash, which will change how addresses are precomputed using CREATE2:

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccountFactory.sol#L45-L47

45:         address expectedAddr = address(
46:             uint160(uint256(keccak256(abi.encodePacked(bytes1(0xff), address(this), salt, keccak256(code)))))
47:         );

This means that the protocol will be forced to always use the same reference implementation of the AmbireAccount contract and won't be able to provide a new implementation if they want to update how new wallets should be created. Changing the address in the proxy initcode will change how addresses are precomputed, breaking counterfactuality.

Recommendation

The reference implementation address can be stored in the AmbireAccountFactory contract. The proxy can query its caller (the factory) during initialization to obtain the address of the implementation where it should delegate calls to.

Assessed type

Other

[Ivshti]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[romeroadrian]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

@Ivshti @Picodes

Just to be clear, I'm talking about the base implementation to be used in new deployments, not upgrading already deployed wallets. If you need to add extra functionality to existing wallets you would need to use the fallback handler (proxies are not upgradeable, and the report doesn't refer to this). Note that this would require one transaction per wallet, as

My recommendation is to allow changes to the implementation address that is referenced in the initcode of the deployment. This could be used to deploy new wallets with the new requirements already baked in, instead of deploying a wallet that would still need to be upgraded via fallback handler, all this without breaking counterfactuality.

[Ivshti]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

@Ivshti @Picodes

Just to be clear, I'm talking about the base implementation to be used in new deployments, not upgrading already deployed wallets. If you need to add extra functionality to existing wallets you would need to use the fallback handler (proxies are not upgradeable, and the report doesn't refer to this). Note that this would require one transaction per wallet, as

My recommendation is to allow changes to the implementation address that is referenced in the initcode of the deployment. This could be used to deploy new wallets with the new requirements already baked in, instead of deploying a wallet that would still need to be upgraded via fallback handler, all this without breaking counterfactuality.

We’re aware of this, and we are not going to do it because it adds an extra SSTORE SLOAD (my bad) and we expect upgradability to be really rarely required. Plus, it adds an extra vector of security issues like being able to brick a wallet way easier

[romeroadrian]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

@Ivshti @Picodes Just to be clear, I'm talking about the base implementation to be used in new deployments, not upgrading already deployed wallets. If you need to add extra functionality to existing wallets you would need to use the fallback handler (proxies are not upgradeable, and the report doesn't refer to this). Note that this would require one transaction per wallet, as My recommendation is to allow changes to the implementation address that is referenced in the initcode of the deployment. This could be used to deploy new wallets with the new requirements already baked in, instead of deploying a wallet that would still need to be upgraded via fallback handler, all this without breaking counterfactuality.

We’re aware of this, and we are not going to do it because it adds an extra SSTORE and we expect upgradability to be really rarely required. Plus, it adds an extra vector of security issues like being able to brick a wallet way easier

Sorry, what do you mean by an extra SSTORE? I think we may be talking about different things here. The implementation address can still be defined in the deployed bytecode without using storage.

[Ivshti]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

@Ivshti @Picodes Just to be clear, I'm talking about the base implementation to be used in new deployments, not upgrading already deployed wallets. If you need to add extra functionality to existing wallets you would need to use the fallback handler (proxies are not upgradeable, and the report doesn't refer to this). Note that this would require one transaction per wallet, as My recommendation is to allow changes to the implementation address that is referenced in the initcode of the deployment. This could be used to deploy new wallets with the new requirements already baked in, instead of deploying a wallet that would still need to be upgraded via fallback handler, all this without breaking counterfactuality.

We’re aware of this, and we are not going to do it because it adds an extra SSTORE and we expect upgradability to be really rarely required. Plus, it adds an extra vector of security issues like being able to brick a wallet way easier

Sorry, what do you mean by an extra SSTORE? I think we may be talking about different things here. The implementation address can still be defined in the deployed bytecode without using storage.

I'm sorry, I meant extra SLOAD on every transaction. With the fallback handler we only have an extra SLOAD if you execute a function which doesn't exist

[romeroadrian]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

@Ivshti @Picodes Just to be clear, I'm talking about the base implementation to be used in new deployments, not upgrading already deployed wallets. If you need to add extra functionality to existing wallets you would need to use the fallback handler (proxies are not upgradeable, and the report doesn't refer to this). Note that this would require one transaction per wallet, as My recommendation is to allow changes to the implementation address that is referenced in the initcode of the deployment. This could be used to deploy new wallets with the new requirements already baked in, instead of deploying a wallet that would still need to be upgraded via fallback handler, all this without breaking counterfactuality.

We’re aware of this, and we are not going to do it because it adds an extra SSTORE and we expect upgradability to be really rarely required. Plus, it adds an extra vector of security issues like being able to brick a wallet way easier

Sorry, what do you mean by an extra SSTORE? I think we may be talking about different things here. The implementation address can still be defined in the deployed bytecode without using storage.

I'm sorry, I meant extra SLOAD on every transaction. With the fallback handler we only have an extra SLOAD if you execute a function which doesn't exist

No, I believe the address can still be hardcoded in the proxy deployed bytecode (meaning it won't need an extra SLOAD) while not being hardcoded in the initcode (allowing flexibility while still preserving counterfactuality).

[Ivshti]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

@Ivshti @Picodes Just to be clear, I'm talking about the base implementation to be used in new deployments, not upgrading already deployed wallets. If you need to add extra functionality to existing wallets you would need to use the fallback handler (proxies are not upgradeable, and the report doesn't refer to this). Note that this would require one transaction per wallet, as My recommendation is to allow changes to the implementation address that is referenced in the initcode of the deployment. This could be used to deploy new wallets with the new requirements already baked in, instead of deploying a wallet that would still need to be upgraded via fallback handler, all this without breaking counterfactuality.

We’re aware of this, and we are not going to do it because it adds an extra SSTORE and we expect upgradability to be really rarely required. Plus, it adds an extra vector of security issues like being able to brick a wallet way easier

Sorry, what do you mean by an extra SSTORE? I think we may be talking about different things here. The implementation address can still be defined in the deployed bytecode without using storage.

I'm sorry, I meant extra SLOAD on every transaction. With the fallback handler we only have an extra SLOAD if you execute a function which doesn't exist

No, I believe the address can still be hardcoded in the proxy deployed bytecode (meaning it won't need an extra SLOAD) while not being hardcoded in the initcode (allowing flexibility while still preserving counterfactuality).

ok but how is the proxy going to check if the implementation is updated on each transaction?

[romeroadrian]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

@Ivshti @Picodes Just to be clear, I'm talking about the base implementation to be used in new deployments, not upgrading already deployed wallets. If you need to add extra functionality to existing wallets you would need to use the fallback handler (proxies are not upgradeable, and the report doesn't refer to this). Note that this would require one transaction per wallet, as My recommendation is to allow changes to the implementation address that is referenced in the initcode of the deployment. This could be used to deploy new wallets with the new requirements already baked in, instead of deploying a wallet that would still need to be upgraded via fallback handler, all this without breaking counterfactuality.

We’re aware of this, and we are not going to do it because it adds an extra SSTORE and we expect upgradability to be really rarely required. Plus, it adds an extra vector of security issues like being able to brick a wallet way easier

Sorry, what do you mean by an extra SSTORE? I think we may be talking about different things here. The implementation address can still be defined in the deployed bytecode without using storage.

I'm sorry, I meant extra SLOAD on every transaction. With the fallback handler we only have an extra SLOAD if you execute a function which doesn't exist

No, I believe the address can still be hardcoded in the proxy deployed bytecode (meaning it won't need an extra SLOAD) while not being hardcoded in the initcode (allowing flexibility while still preserving counterfactuality).

ok but how is the proxy going to check if the implementation is updated on each transaction?

It can be retrieved from the factory. Proxy initcode queries the caller for the address and writes the result in the returned bytecode to be deployed.

[Ivshti]

@Picodes we strongly disagree with the severity here, this is absolutely by design. The fallback handler is intended to provide user-initiated upgradability for cases where upgradability is absolutely needed (eg another standard like ERC-4337 emerging which requires on-chain implementations to have an extra method). But other than that, Ambire accounts are intended to be non-upgradable.

@Ivshti @Picodes Just to be clear, I'm talking about the base implementation to be used in new deployments, not upgrading already deployed wallets. If you need to add extra functionality to existing wallets you would need to use the fallback handler (proxies are not upgradeable, and the report doesn't refer to this). Note that this would require one transaction per wallet, as My recommendation is to allow changes to the implementation address that is referenced in the initcode of the deployment. This could be used to deploy new wallets with the new requirements already baked in, instead of deploying a wallet that would still need to be upgraded via fallback handler, all this without breaking counterfactuality.

We’re aware of this, and we are not going to do it because it adds an extra SSTORE and we expect upgradability to be really rarely required. Plus, it adds an extra vector of security issues like being able to brick a wallet way easier

Sorry, what do you mean by an extra SSTORE? I think we may be talking about different things here. The implementation address can still be defined in the deployed bytecode without using storage.

I'm sorry, I meant extra SLOAD on every transaction. With the fallback handler we only have an extra SLOAD if you execute a function which doesn't exist

No, I believe the address can still be hardcoded in the proxy deployed bytecode (meaning it won't need an extra SLOAD) while not being hardcoded in the initcode (allowing flexibility while still preserving counterfactuality).

ok but how is the proxy going to check if the implementation is updated on each transaction?

It can be retrieved from the factory. Proxy initcode queries the caller for the address and writes the result in the returned bytecode to be deployed.

I'm sorry, I really don't understand the point of this. If you're talking about new accounts, then their bytecode is set by the front-end and passed to the factory anyway, there's no need for the factory to do anything to support that type of upgradability. For already existing accounts, the only way to do it implies an SLOAD