Lines of code

Vulnerability details

Issue

M-05: AmbireAccount implementation can be destroyed by privileges

Mitigation

https://github.com/AmbireTech/ambire-common/commit/3a0a4d9b24c96d816fb5819efe1e5f4dc57d7835

Assessment of Mitigation

Issue mitigated

Comments

After the mitigation, the constructor of the AmbireAccount contract is removed, which is also confirmed by the following code comment for this contract. Because the AmbireAccount contract does not have a constructor now, its deployer cannot maliciously or accidentally grant a privilege to an address. In this case, since such address is not privileged, it cannot call the AmbireAccount contract's functions that require a sufficient privilege; for example, such address that does not have a sufficient privilege cannot call the AmbireAccount.executeBySender function below for granting a privilege to a malicious fallback handler, which can destruct the AmbireAccount implementation contract, anymore. Thus, the corresponding issue is mitigated.

https://github.com/AmbireTech/ambire-common/blob/f56e7dcaaa4ff8950b39fe6d5bced165d0f1c99f/contracts/AmbireAccount.sol#L9-L12

contract AmbireAccount {
    // @dev We do not have a constructor. This contract cannot be initialized with any valid `privileges` by itself!
    // The indended use case is to deploy one base implementation contract, and create a minimal proxy for each user wallet, by
    // using our own code generation to insert SSTOREs to initialize `privileges` (IdentityProxyDeploy.js)

https://github.com/AmbireTech/ambire-common/blob/f56e7dcaaa4ff8950b39fe6d5bced165d0f1c99f/contracts/AmbireAccount.sol#L200-L205

    function executeBySender(Transaction[] calldata calls) external payable {
        require(privileges[msg.sender] != bytes32(0), 'INSUFFICIENT_PRIVILEGE');
        executeBatch(calls);
        // again, anti-bricking
        require(privileges[msg.sender] != bytes32(0), 'PRIVILEGE_NOT_DOWNGRADED');
    }

code-423n4 / 2023-06-ambire-mitigation-findings