Reach out for an audit or to learn more about Macro
or Message on Telegram

Infinex A-1

Security Audit

May 10, 2024

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Infinex's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team in 5 parts (*) (**):


* Audit 1d is not included as it was referred to a different set of contracts.
** Audit findings that do not correspond to the hashed files described below have been omitted as some contracts have not been included in the v1 implementation.

The purpose of this audit is to review the source code of certain Infinex Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
Critical 1 - - 1
High 3 - - 3
Medium 2 1 - 1
Low 3 - - 3
Code Quality 8 1 1 6
Informational 3 - - -

Infinex was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Trust Model, Assumptions, and Accepted Risks (TMAAR)

As part as a comprehensive review, security included not only that user funds would be secure from external actors, but also from Infinex acting maliciously.

Infinex Governance

Protocol DAO that has control over contract implementation upgrades and configurations as the Owner role:

Infinex Users

Users control their accounts with the following keys:

Sudo Keys

Super user set of addresses, initially set during the account creation as the Account owner but can be expanded to multiple Sudo key entities. Essentially acts as the highest authority and can perform most actions such as set other Sudo keys, setup Operational keys, setup the Recovery key, upgrade the Account's implementation, upgrade Configurations from the Protocol beacon, and perform withdrawals.

Operation Keys

Multiple Operation Keys can be added by any Sudo key and they can only execute intra-account bridging within different chains.

Recovery Keys and Recovery Address

A Sudo Key user can set the immutable Recovery Address once and enable Recovery Keys to recover USDC and tokens to this specific address. It is worth to note that the immutability of this address is only ensured within this Account's implementation as the logic might be migrated and allowed to change this variable. Multiple Recovery keys can be added by any Sudo key, this special key actor can recover the USDC balance to a specific Circle CCTP Domain and ERC20 tokens, only if a Fund Recovery Address has been set as mentioned before.

External Assumptions

It is assumed that Infinex governance will set withdrawal fees, and that all account upgrades and beacon updates that accounts can opt into will be beneficial for users.

Account Assumptions

Fees

Current account implementations have a maximum withdrawal fee hardcoded in the Account of 50 USDC tokens. Withdrawals fees of different ERC20 tokens are also charged in USDC.

Source Code

The following source code was reviewed during the audit:

  • Scope
  • Note: Currently the referenced repository is private, but there are plans to make it public in the future. The contracts reviewed can be viewed here, as version 1.0.0 O2 , and have been verified on the primary block explorer for each supported chain.

    The following contracts were audited (*):

    Contract SHA256
    ./src/Initializable.sol

    c2ba091b45bedcb2710b727bca1280769fcd9ec10267611379c72998a70c5b64

    ./src/accounts/AccountFactory.sol

    f152119fa2802a63f6d80816cc4f43c00902c7b2e49ba831237a595395dd6357

    ./src/accounts/modules/AccountUtilsModule.sol

    eaba1552dc6d860ec17773d6248601b1e93c551e27e7262bbebbc6db814d4417

    ./src/accounts/modules/BaseModule.sol

    06eaf3cfa23bbe45d2620a5ed914f15effa122b19c864bc3fcea53e08ff56509

    ./src/accounts/modules/BridgingModule.sol

    e1e02bf5dc9fad21689e9a67e75f4fdefd65c77bb830e406e20db162d83146d7

    ./src/accounts/modules/RecoveryModule.sol

    fecb1e896f5e2b55adf6a5656aa4255d0ad63d3e4b8ad7577edb61b24a19c76a

    ./src/accounts/modules/WithdrawModule.sol

    59329b46074d605341a63ffb67400508c8763eaa8ca2e74a413fc2cbb958fcae

    ./src/accounts/storage/Account.sol

    c136d59774bfc6a52e6ce162d9e1e491882e253e8ea4eb376a83cea92a515ea6

    ./src/accounts/storage/Bridge.sol

    cc8f253b656a46d605b53e1e19664448d4560d412032913383641d7861abe188

    ./src/accounts/storage/EIP712.sol

    00922e211d9cb73dbfb374910cf05cc52b8c11618e3d903d730e9c7d4a7a2e1d

    ./src/accounts/storage/Recovery.sol

    a4a6392ec56f3c3d1416a38e5a9e12ba332933fb73213024ac8cdafe8a99d8cf

    ./src/accounts/storage/SecurityKeys.sol

    e0a356558af9c23344a6db8433c35fe440ceb91112f57bac28479befacf74f1e

    ./src/accounts/storage/Withdrawal.sol

    7a24b16a5a9524557956a494de8eefa8a7f0f4d2afffff5d40afd3afabd08764

    ./src/accounts/utils/AccountConstants.sol

    e6f4420118e4b841d54a2e31f65af8905c86f01068bba4270efdca2f867c0e98

    ./src/accounts/utils/RequestTypes.sol

    47f7d9e0aef9cb95b439fdff5d01c232528650d5f62d29c86f4fcb67135f8c42

    ./src/accounts/utils/SecurityModifiers.sol

    2e75ab375d273880ec4349a1a29c2574121a2a83b45718589ea00cac971c63dc

    ./src/accounts/utils/WithdrawUtil.sol

    c647e1b8f2784424923bcb207d2f231b5b1963fa63723c6c911cf3e2d6cbeb27

    ./src/beacons/InfinexProtocolConfigBeacon.sol

    90fa65add6293fb7d90fbd4a6192cc746751e3646f8026414e5fba4c59f1a50d

    ./src/forwarder/ERC2771Context.sol

    9d292ad949fc252ef9836f4d9dacf7d09590b0b41d6d841a9f97d0af0b5a0049

    ./src/forwarder/InfinexERC2771Forwarder.sol

    9b5ee721951882983675ee932c64e91e79be0a0ce07530b592894994341ed497

    ./src/integrations/BridgeIntegrations.sol

    f653b5e2642b5c73b4dee3b8377872411b6ee57591b86883b0746eed79e23d0e

    ./src/interfaces/accounts/IAccountFactory.sol

    4705e8cf64cf44304b61a6c18480b0658c7b54d686e63682bb82980439c31323

    ./src/interfaces/accounts/IAccountUtilsModule.sol

    450c229079e6be901e728bab86d50a1f4f685a94a2090dfa7b3d6924a16a5889

    ./src/interfaces/accounts/IBaseModule.sol

    23a682f3f62b5ded288b044260aa63813290f1a3aab8c28c9b9dd59a5b31b590

    ./src/interfaces/accounts/IBridgingModule.sol

    31b435abebfc990987cfd8a89cc3ed4897ddc4cb88a8a3ac2eeb7bfd78043be9

    ./src/interfaces/accounts/IRecoveryModule.sol

    89369194578d70bfd74a03dad5687b7aa5a6c9e466a314f1d2421743c0681dde

    ./src/interfaces/accounts/IWithdrawModule.sol

    2a637559b7238839b7d401d566c323b8d83f7890de3eaa6491c2ba32f36b13c8

    ./src/interfaces/beacons/IInfinexProtocolConfigBeacon.sol

    89ddb97a3464b0c3c91b8b311d266b008fa9216902f2b092b141555cc4c5bf43

    ./src/libraries/DecimalScaling.sol

    681eed80d6482032e273a7a85825cc5e4aee6c64b54b167b6528245e03e3b840

    ./src/libraries/Error.sol

    ec69fd445c45605ba659c356d9ae791e256f95d2016da96e353e6879d05b6f7c

    ./src/libraries/SolanaUtils.sol

    4fd2c604b220bc016e0d441bac3566af17aa9e3042c64b4dcc20446fc7258b2c

    ./src/proxy/InfinexProxy.sol

    3d5fd4f57ca1681900930fb02dbc514b657af3d56764b4e76e6dad0873a7822a

    ./src/proxy/InitialProxyImplementation.sol

    90ece440dae4b48932a52e9b66918969c02527a8c7accf3aee0144baaa53326a

    * File hashes shown above are the final version of the reviewed contracts, corresponding to the final commit hash.

    Issue Descriptions and Recommendations

    Click on an issue to jump to it, or scroll down to see them all.

    Security Level Reference

    We quantify issues in three parts:

    1. The high/medium/low/spec-breaking impact of the issue:
      • How bad things can get (for a vulnerability)
      • The significance of an improvement (for a code quality issue)
      • The amount of gas saved (for a gas optimization)
    2. The high/medium/low likelihood of the issue:
      • How likely is the issue to occur (for a vulnerability)
    3. The overall critical/high/medium/low severity of the issue.

    This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

    Severity Description
    (C-x)
    Critical

    We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

    (H-x)
    High

    We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

    (M-x)
    Medium

    We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

    (L-x)
    Low

    The risk is small, unlikely, or may not relevant to the project in a meaningful way.

    Whether or not the project wants to develop a fix is up to the goals and needs of the project.

    (Q-x)
    Code Quality

    The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

    (I-x)
    Informational

    Warnings and things to keep in mind when operating the protocol. No immediate action required.

    (G-x)
    Gas Optimizations

    The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

    Issue Details

    C-1

    Accounts can be deployed with malicious keys

    Topic
    Protocol Design
    Status
    Impact
    Critical
    Likelihood
    High
    This fix was completed during a pre-production audit and is fixed in the current codebase. Now the initial `_sudoKey` required to create an Account is used as the `salt` to ensure the Account deployed to that deterministic address is controlled by it.

    The way Infinex’s user flow works is users deploy a deposit account on layer 1 Ethereum using the AccountFactory, providing a seed that can be used on the target layer 2 chains AccountFactory to create a trading account with the same address, allowing the deposit account to bridge funds to the other chain using address(this).

    /**
     * @notice Bridge USDC tokens from Layer 1 to Layer 2 via the circle bridge.
     * @dev The entire balance of the contract will be bridged.
     */
    function bridgeUSDCtoL2() external {
        AccountStorage.Data storage data = AccountStorage.getStorage();
        uint256 amount = IERC20(data.circleBridgeUSDCTokenAddress).balanceOf(address(this));
        IERC20(data.circleBridgeUSDCTokenAddress).approve(data.circleBridge, amount);
    
        emit USDCBridgedToL2(data.circleBridgeUSDCTokenAddress, amount);
        ITokenMessenger(data.circleBridge).depositForBurn(
            amount,
            data.circleBridgeDestinationDomain,
            // The circle bridge expects the mint recipient address as a bytes32
            bytes32(uint256(uint160(address(this)))),
            data.circleBridgeUSDCTokenAddress
        );
    }
    

    Reference: DepositAccountImplementation.sol#L23-L40

    However, since the salt passed into the AccountFactory’s createAccount() function is the only parameter that determines the address of the account on either chain. A malicious user can either frontrun its creation or see a deposit account created on mainnet, and respond by creating a trading account on the L2 chain, using the same salt but providing with their own keys. This still creates an account with the expected address, but the keys controlling withdrawals and trading are not the same as the intended users.

    /// @dev A new UUPSProxy is deployed using CREATE2 with a default implementation.
    /// This ensures that both create2 calls for Trading and Deposit accounts are identical,
    /// resulting in both accounts receiving the same address.
    newAccount = Create2.deploy(0, _salt, _getProxyBytecode(initialProxyImplementation));
    

    Reference: AccountFactory.sol#L109-L112

    This allows attackers to gain control of corresponding L2 trading accounts or even L1 deposit accounts, allowing user funds deposited and/or bridged to compromised accounts they don’t control.

    Remediations to Consider

    Instead of only using the provided _salt to determine the address of deployed accounts, hash the included keys together with the provided _salt and use that as the final salt when deploying. This will ensure that deployed accounts of the same address are initially controlled by the intended users, preventing user assets from being stolen.

    H-1

    Missing fund recovery guard

    Topic
    Spec
    Status
    Impact
    High
    Likelihood
    High
    This fix was completed during a pre-production audit and is fixed in the current codebase. Checks were added to verify that the recovery guard is active prior to recovers.

    With the addition of the RecoveryModule, it allows users to recover funds to a set recovery address, or withdraw them back to a account on a default chain, currently base.

    However, the StWStakingRewards contract assumes that funds can not be removed from the account until after the rewards period has ended, otherwise users would receive governance points for assets they no longer have “staked” in their account. Currently there is no guard preventing fund recovery, which would allow users to withdraw their funds even if they are staked. The documentation mentions a “global allow funds recovery” that would prevent funds from being recovered until set to true, but this has not been put in place. There is a public variable named fundsRecoveryActive in the InfinexProtocolConfigBeacon that can be set by it’s owner:

    /**
    * @notice Sets the funds recovery flag to active.
    * @dev Initially only the owner can call this. After 90 days, it can be activated by anyone.
    */
    function setFundsRecoveryActive() external {
        if (owner() != _msgSender()) {
            if (block.timestamp - CREATED_AT < 90 days) {
                revert Error.FundsRecoveryActivationDeadlinePending();
            }
        }
        emit FundsRecoveryStatusSet(true);
        fundsRecoveryActive = true;
    }
    

    Reference: InfinexProtocolConfigBeacon.sol#L289-L301

    This value seems to be the guard mentioned in the docs, but it is not referenced anywhere in the scope, and in particular not used to prevent early fund recovery in the recoveryModule’s functions.

    Notably, the function recoverToken() allows any token to be withdrawn by either a Sudo key or the recovery key, which would allow all assets to be moved out, even if they are staked with the StWStakingRewards contract:

    /**
     * @notice Recovers all of the specified token to to the funds recovery address on this chain
     * @dev requires the sender to be the origin key or recovery key
     * @param _token The token to recover
     */
    function recoverToken(address _token) external requiresOriginOrRecoveryKey nonReentrant {
        if (Recovery._getFundsRecoveryAddress() == address(0)) revert Error.NullAddress();
        _withdraw(
            _token,
            Recovery._getFundsRecoveryAddress(),
            DecimalScaling.scale(IERC20(_token).balanceOf(address(this)), IERC20Metadata(_token).decimals())
        );
    }
    

    Reference: RecoveryModule.sol#L80-L92

    Remediations to Consider

    Consider ensuring the value of fundsRecoveryActive in the InfinexProtocolConfigBeacon is set to true for each relevant function in the RecoveryModule to prevent funds from moving out of accounts before they should be able to.

    H-2

    AccountFactory upgrades could lock or steal funds sent to un-deployed accounts

    Topic
    Spec
    Status
    Impact
    Critical
    Likelihood
    Low
    This fix was completed during a pre-production audit and is fixed in the current codebase. The current Account factory is not upgradable, notably governance can disable account creation before the account is created in other chains, it is assumed that the DAO won't act maliciously.

    AccountFactory.sol is upgradeable, and can be upgraded by Infinex governance. If it is upgraded in a way that makes it impossible to deploy accounts to the same addresses as the previous factory implementation, assets sent or bridged before deploying the corresponding L2 account would be inaccessible. This could likely be the case if new initialization parameters are added to accounts, which would need to be included in the create2 salt when the factory deploys them, thus changing the resulting addresses.

    In the case where Infinex governance is compromised or acts malicious, it could upgrade the account factory to allow the creation of arbitrary contracts to addresses with funds previously sent, since the salt required to generate the address would be known.

    Remediations to Consider

    The Account Factory could be made to be not upgradeable, and in the case where there are desired changes to the way accounts are deployed, new factories could be deployed across all chains with the same address. This would ensure that all account deployments will remain supported, and funds sent before deployment will always be recoverable by the intended keys.

    H-3

    Bridging USDC to Solana using CCTP using the wrong destination address

    Topic
    Bridge integration
    Status
    Impact
    High
    Likelihood
    High

    According to CCTP documentation, when bridging tokens from a non-Solana chain to Solana, the mintRecipient address should be the USDC-associated token account. However, in bridgeUSDCWithCCTPSolana(), the function is using the _solanaAccount parameter as a destination address.

        function bridgeUSDCWithCCTPSolana(
            uint256 _amount,
            bytes32 _solanaAccount,
            bytes32 _solanaTokenAccount,
            uint8 _accountBump,
            uint8 _tokenAccountBump
        ) external requiresAuthorizedOperationsParty {
                    ...
                        
            _bridgeUSDCWithCCTP(_amount, _solanaAccount, solanaDestinationDomain);
        }
    

    Reference: BridgingModule.sol#L83-121

    Remediations to Consider:

    Consider passing the _solanaTokenAccount as a destination address.

    M-1

    Wormhole bridging doesn’t work when fee is enabled

    Topic
    Bridging
    Status
    Acknowledged
    Impact
    High
    Likelihood
    Low

    RecoveryModule.bridgeUSDCWithWormholeForRecovery allows for bridging USDC tokens from current chain to Base. The actual call to the wormhole bridge happens in BridgeIntegration._bridgeUSDCWithWormhole:

    ICircleIntegration(wormholeCircleBridge).transferTokensWithPayload(transferParameters, 0, abi.encode(msg.sender));
    

    Reference: BridgeIntegrations.sol#L86

    The above call to transferTokensWithPayload works fine as long as their are no fees charged for sending a message. If fees are enabled for a certain chain, the call will fail as no fee value is passed.

    This is also mentioned in the comments for transferTokensWithPayload function:

     * @dev reverts if:
     * - user passes insufficient value to pay Wormhole message fee
    

    Reference: CircleIntegration.sol#L36

    Note that currently fees are only enabled for Solana and no fees are charged for any EVM chains.

    However, this can change in the future as also mentioned in the wormhole documentation:

    Currently there are no fees to publish a message (with the exception of publishing on Solana) but this is not guaranteed to always be the case in the future.
    

    Remediation to Consider

    Pass the actual fee value returned by IWormhole.messageFee() to the transferTokensWithPayload call.

    Response by Infinex

    As part of our partnership with Wormhole, we will be notified if the fee is set. If the fee is set and we are not notified, we would want these calls to fail.

    M-2

    Trusted forwarder update is not controlled by the user

    Topic
    Trust Assumption
    Status
    Impact
    Medium
    Likelihood
    Low
    This fix was completed during a pre-production audit and is fixed in the current codebase. Now, if users remove the trusted forwarder, when upgrading the trusted forwarder from the new config beacon won't be added.

    While upgrading the protocol beacon parameters, the user passes the expected beacon address to the upgradeProtocolBeaconParameters function to verify it’s the implementation address to be used as a new protocol beacon.

    function upgradeProtocolBeaconParameters(address _newInfinexProtocolConfigBeacon) external requiresStrongDeviceKey {
        Account.Data storage accountData = Account.getStorage();
        IInfinexProtocolConfigBeacon protocolConfigBeacon = Account._infinexProtocolConfig();
        address latestInfinexProtocolConfigBeacon = protocolConfigBeacon.latestInfinexProtocolConfigBeacon();
    
        if (latestInfinexProtocolConfigBeacon == address(protocolConfigBeacon)) {
            revert Error.SameAddress();
        }
        if (latestInfinexProtocolConfigBeacon != _newInfinexProtocolConfigBeacon) {
            revert Error.ImplementationMismatch(_newInfinexProtocolConfigBeacon, latestInfinexProtocolConfigBeacon);
        }
    
        ERC2771Context._removeTrustedForwarder(protocolConfigBeacon.trustedForwarder());
        ERC2771Context._addTrustedForwarder(
            IInfinexProtocolConfigBeacon(latestInfinexProtocolConfigBeacon).trustedForwarder()
        );
    
        emit AccountInfinexProtocolBeaconImplementationUpgraded(latestInfinexProtocolConfigBeacon);
        accountData.infinexProtocolConfigBeacon = latestInfinexProtocolConfigBeacon;
    }
    

    Reference: UtilsModule.sol#L313-332

    However, within this call, the trusted forwarder is also being updated. The trusted forwarder address can’t be predicted by the user, and it would depend on the beacon implementation. If this parameter is updatable, the user could end up with a different trusted forwarder than expected.

    Remediations To Consider

    Adding an expected _newTrustedForwarder address and asserting against the new protocol config beacon trusted forwarder to be added, or having a separate function to update the trusted forwarder.

    Also, consider including the trusted forwarder address added to the AccountInfinexProtocolBeaconImplementationUpgraded event.

    L-1

    Mislabeling wormhole chain ids as domains

    Topic
    Data Model
    Status
    Impact
    Low
    Likelihood
    Low
    This fix was completed during a pre-production audit and is fixed in the current codebase.

    In the RecoveryModule's bridgeUSDCWithWormholeForRecovery() function, it allows a trusted recovery keeper to bridge the accounts USDC to the default chain, which is base. The default chain to bridge to is defined by the _wormholeDefaultDestinationDomain value stored in the accounts bridge struct set on initialization.

     BridgeIntegrations._bridgeUSDCWithWormhole(
          maxAmount, bytes32(uint256(uint160(address(this)))), Bridge._wormholeDefaultDestinationDomain()
      );
    

    Reference: RecoveryModule.sol#L75-L77

    This “domain” is passed into the internal _bridgeUSDCWithWormhole() function as the _destinationDomain, which is also validated by calling _validateWormholeDestinationDomain():

    function _bridgeUSDCWithWormhole(uint256 _amount, bytes32 _destinationAddress, uint16 _destinationDomain) internal {
        _checkExceedsMinBridgeAmount(_amount);
        if (!_validateWormholeDestinationDomain(_destinationDomain)) revert Error.InvalidDestinationDomain();
        ...
    

    Reference: BridgeIntegrations.sol#L70-L72

    function _validateWormholeDestinationDomain(uint16 _domain) internal view returns (bool isValid) {
        if (ICircleIntegration(Bridge._wormholeCircleBridge()).getDomainFromChainId(_domain) == 0) return false;
        return true;
    }
    

    Reference: BridgeIntegrations.sol#L103-L106

    Notice the validation call made to the bridge is getDomainFromChainId(), as this “domain” is really a chain id.

    Although this implementation is setup correctly as a chain id aside, from the mislabeling, there could be issues in setting up these values incorrectly by inputting the domain rather than the expected chainId for the _wormholeDefaultDestinationDomain value. This could lead to issues for accounts that may require an upgrade, since the Bridge struct is only written to on the account’s initialization.

    Remediations to Consider

    Rename the _wormholeDefaultDestinationDomain in the accounts Bridge struct, and in the InfinexProtocolConfigBeacon’s BridgeConfiguration struct as a chain id, rather than a domain, and replace the input parameter names of _bridgeUSDCWithWormhole() and _validateWormholeDestinationDomain() to _destinationChainId and _chainId respectively, and rename _validateWormholeDestinationDomain() to _validateWormholeDestinationChainId().

    L-2

    _computeScaledAmount() takes arbitrary tokens but checks USDC bridge max amount

    Topic
    Decimals
    Status
    Impact
    Low
    Likelihood
    Low

    BridgeModule.sol implements _computeScaledAmount() to account for any passed token decimals and verify the maximum bridgable amount and balance.

    function _computeScaledAmount(uint256 _amount, address _tokenAddress) internal returns (uint256) {
        uint256 scaledAmount = uint256(DecimalScaling.scaleTo(int256(_amount), IERC20Metadata(_tokenAddress).decimals()));
    
        if (scaledAmount > BridgeIntegrations._getBridgeMaxAmount()) revert Error.BridgeMaxAmountExceeded();
    
        if (scaledAmount > IERC20(_tokenAddress).balanceOf(address(this))) revert Error.InsufficientBalance();
    
        return scaledAmount;
    }
    

    Reference: BridgingModule.sol#L161-169

    However, the function _getBridgeMaxAmount() fetches the maximum bridge amount of the hardcoded USDC token, which counts with 6 decimals, with the current value on the Base chain being set to 1_000_000_000_000, which equals 1_000_000 tokens with 6 decimals.

    function _getBridgeMaxAmount() internal view returns (uint256) {
        return ITokenMinter(Bridge._circleMinter()).burnLimitsPerMessage(Bridge._USDC());
    }
    

    Reference: BridgingIntegrations.sol#L125-127

    If the function _computeScaledAmount is used with a token with a different number of decimals, the max bridge amount will not be accurate, and the function could revert incorrectly. For example, if used with USDT, an 18-decimal token, the maximum limit would translate to 0,000001 USDT tokens and revert if higher than this.

    Remediations to Consider:

    Consider moving the _getBridgeMaxAmount() check outside the internal _computeScaledAmount() function or implementing logic only for the USDC token.

    L-3

    Ether transfer may revert with out of gas

    Topic
    Use cases
    Status
    Impact
    Low
    Likelihood
    Medium

    _withdrawEther function in WithdrawUtil uses .transfer() built-in method to transfer native assets to the withdrawal address. This hardcodes 2300 gas to the call and can revert for multi-sig addresses or smart contracts accounts.

    function _withdrawEther(address _withdrawalAddress, uint256 _amount) internal {
        _takeUSDCWithdrawalFee();
        emit WithdrawalEtherExecuted(_withdrawalAddress, _amount);
        payable(_withdrawalAddress).transfer(_amount);
    }
    

    Reference: WithdrawUtil.sol#L112-116

    Remediations to Consider:

    Consider using a low-level .call() with the corresponding msg.value and verifying the result.

    Q-1

    Redundant token scaling when recovering tokens

    Topic
    Quality
    Status
    Wont Do
    Quality Impact
    Low

    Note: This issue is from 1e audit.

    In the RecoveryModule, when recoverToken() is called, it calls the internal function _withdraw() and scales the accounts balance of the token to 18 decimals.

     _withdraw(
          _token,
          Recovery._getFundsRecoveryAddress(),
          DecimalScaling.scale(IERC20(_token).balanceOf(address(this)), IERC20Metadata(_token).decimals())
      );
    

    Reference: RecoveryModule.sol#L87-L91

    This is then immediately is scaled back to the tokens original decimals, effectively undoing the prior scale up.

    function _withdraw(address _token, address _destinationAddress, uint256 _amount) internal {
        // amount is in 18 decimals, scale to token decimals
        uint256 scaledAmount = DecimalScaling.scaleTo(_amount, IERC20Metadata(_token).decimals());
        uint256 amountAfterFee = scaledAmount;
        ...
    

    Reference: WithdrawUtil.sol#L63-L66

    Doing so may make sense to do if other functions used _withdraw(), and required the input to be in 18 decimals, but currently recoverToken() is the only function using this method.

    Remediations to Consider

    Adjust _withdraw() to take the _amount value in the corresponding tokens original decimals to prevent redundant scaling.

    Response by Infinex

    We have plans to use the _withdraw() function for other purposes in the future.

    Q-2

    NatSpec and comments

    Topic
    Quality
    Status
    Quality Impact
    Low
    This fix was completed during a pre-production audit and is fixed in the current codebase.
    • Additional 'to' in natspec comment in recoverToken().
    • Missing NatSpec in requiresTrustedRecoveryKey modifier.
    Q-3

    Missing event for state-changing function

    Topic
    Quality
    Status
    Quality Impact
    Low
    This fix was completed during a pre-production audit and is fixed in the current codebase.

    The function RecoveryModule.setFundsRecoveryAddress is called to set the recovery address. This function is currently not emitting any event, which is recommended for every state-changing function.

    Remediation to Consider

    Add an appropriate event to the setFundsRecoveryAddress function.

    Q-4

    Redundant check for minimum bridge amount

    Topic
    Quality
    Status
    Acknowledged
    Quality Impact
    Low

    In RecoveryModule.recoverUSDCToEVMChain, it is checked that USDC balance is greater than the minimum bridge amount:

    if (balance < protocolConfigBeacon.getMinimumUSDCBridgeAmount()) revert Error.InsufficientBalance();
    

    Reference: RecoveryModule.sol#L118

    The function recoverUSDCToEVMChain internally calls WithdrawUtil._withdrawUSDCToChain which in turn calls BridgeIntegrations._bridgeUSDCWithCCTP(amountAfterFee, …).

    In _bridgeUSDCWithCCTP, the minimum bridge amount is again checked by calling _checkExceedsMinBridgeAmount:

    function _checkExceedsMinBridgeAmount(uint256 _amount) internal view {
      if (_amount < Account._infinexProtocolConfig().getMinimumUSDCBridgeAmount()) revert Error.InsufficientBalance();
    }
    

    Reference: BridgeIntegrations.sol#L117-L119

    The only difference between those checks is that the latter one takes the actual amount being bridged, meaning the balance subtracted by the fee.

    Remediation to Consider

    To improve code readability and reduce gas costs, remove the getMinimumUSDCBridgeAmount() check from RecoveryModule.recoverUSDCToEVMChain().

    Q-5

    Unused event

    Topic
    Quality
    Status
    Quality Impact
    Low
    This fix was completed during a pre-production audit and is fixed in the current codebase.

    In WithdrawUtil.sol, the following event is defined but not used anywhere in the code:

    event EthereumWithdrawalStarted(address indexed token, address indexed EthereumAddressTo, uint256 amount);
    

    Remediation to Consider

    Remove the above event definition from the code.

    Q-6

    Cache external parameter in memory

    Topic
    Optimization
    Status
    Quality Impact
    Low

    recoverToken() could cache fundsRecoveryAddress in memory, similar to recoverUSDCToEVMChain().

    function recoverToken(address _token) external requiresAuthorizedRecoveryParty nonReentrant {
        IInfinexProtocolConfigBeacon protocolConfigBeacon = Account._infinexProtocolConfig();
        if (!protocolConfigBeacon.fundsRecoveryActive()) revert Error.FundsRecoveryNotActive();
        if (Recovery._getFundsRecoveryAddress() == address(0)) revert Error.NullAddress();
        _withdrawERC20(
            _token,
            Recovery._getFundsRecoveryAddress(),
            DecimalScaling.scale(IERC20(_token).balanceOf(address(this)), IERC20Metadata(_token).decimals())
        );
    }
    
    /**
     * @notice Recovers all USDC to the funds recovery address on the specified chain via Circle CCTP.
     * @param _destinationCCTPDomain The CCTP domain id of the destination chain.
     * @return amountBridged The amount of USDC bridged in native decimals (6).
     * @return fullBalanceBridged A boolean indicating if the amount to bridge is the full balance of the account.
     * @dev Requires the sender to be either sudo or recovery key.
     */
    function recoverUSDCToEVMChain(uint32 _destinationCCTPDomain)
        external
        requiresAuthorizedRecoveryParty
        nonReentrant
        returns (uint256 amountBridged, bool fullBalanceBridged)
    {
        ...
        address fundsRecoveryAddress = Recovery._getFundsRecoveryAddress();
        if (fundsRecoveryAddress == address(0)) revert Error.NullAddress();
        ...
        amountBridged = _withdrawUSDCToChain(maxAmount, fundsRecoveryAddress, _destinationCCTPDomain);
        ...
    }
    

    Reference: RecoveryModule.sol#L87-123

    Q-7

    Inaccurate comments

    Topic
    Documentation
    Status
    Quality Impact
    Low

    Some Natspec comments in the contracts still refer to an outdated key name convention but should refer to the operation key:

    Additionally, some comments do not reflect the function’s implementation:

    Q-8

    Duplicated internal functions name

    Topic
    Best practice
    Status
    Quality Impact
    Low

    Function _bridgeUSDCWithWormhole() and _bridgeUSDCWithCCTP() are present in both, BridgingModule.sol and BridgeIntegrations.sol contracts. Consider renaming these in one of the contracts to avoid confusion and potentially avoid missing any validation checks from the BridgingModule internal functions.

    I-1

    The fund recovery address should be set to a safe address for each account before adding funding

    Topic
    Informational
    Impact
    Informational

    The fund recovery address is the address that funds will be sent to when recoverToken() is called by either a Sudo key or a set Recovery key. This address can only be set once by a Sudo key:

     /**
     * @notice Set the funds recovery address
     * @param _fundsRecoveryAddress the address for funds recovery.
     * @dev The origin key is required as a modifier to add a recovery address to recovery funds.
     */
    function setFundsRecoveryAddress(address _fundsRecoveryAddress) external requiresOriginKeySender {
        if (Recovery._getFundsRecoveryAddress() != address(0)) revert Error.AddressAlreadySet();
        if (_fundsRecoveryAddress == address(0)) revert Error.NullAddress();
        Recovery._setFundsRecoveryAddress(_fundsRecoveryAddress);
    }
    

    Reference: RecoveryModule#L55-L64

    In the case where a malicious actor gains access to the user's Sudo key, if the funds recovery address is not yet set, then they could set it to a wallet they control and steal all assets in the account. However, if users set this to a safe and secure wallet address before adding funds, there is currently no way for funds to be stolen by a malicious Sudo key, protecting a users funds.

    Response by Infinex

    Although this is not blocked in the smart contracts, we do not show users their deposit address until they have specified their funds recovery address.

    I-2

    Storage corruption breaks access control roles

    Topic
    Implementation Migration
    Impact
    Informational

    In this version of the core Infinex contracts, SecurityKeys storage removed the previous singleton originKey address parameter member from the Data struct, added new mappings, and the sudoKeysCounter variable.

    For accounts that upgraded the previous storage implementation, the following data struct changes were reviewed.

    • 1c version:
    struct Data {
        address originKey;
        mapping(bytes32 => bool) nonces; // Mapping of nonces
        mapping(address => bool) strongDeviceKeys; // Mapping of strong device keys
        mapping(address => uint256) weakDeviceKeys; // Mapping of weak device keys
    }
    
    • 1e version:
    struct Data {
        address originKey;
        mapping(bytes32 => bool) nonces; // Mapping of nonces
        mapping(address => bool) recoveryKeys;
    }
    
    • 1f version:
      struct Data {
          mapping(bytes32 => bool) nonces; // Mapping of nonces
          mapping(address => bool) operationKeys;
          mapping(address => bool) recoveryKeys;
          mapping(address => bool) sudoKeys;
          uint16 sudoKeysCounter;
      }
    

    From 1c to 1e versions, all previously set strongDeviceKeys stored in accounts would be valid recoveryKeys, as the mapping storage pointer would be the same as the previous.

    In 1f migration, since the originKey variable was removed the account storage will change drastically causing, in the struct declaration order:

    • nonces to be reset to a new storage pointer, and since the type hash and domain separator parameters are the same as in previous iterations, previously used and consumed nonces will be valid again, allowing signed transactions to be relayed and replayed.
    • operationKeys mapping could be corrupted depending on the used nonces to be set as true. However, this is not a critical access control role, and it's highly dependent on the value of nonces used as they might not render a malicious address or a valid EOA.
    • sudoKeys will use the storage slot of the previously used weakDeviceKeys in 1c. Furthermore, since the previous variable mapping uses uint256 as a value and used to be set to block.timestamp, all odd numbers set will return true for the given key. weakDeviceKeys would then become sudoKeys.

    Considerations for Upgrades:

    • Keeping deprecated variables in the struct, changing their name to describe this, and adapting new variables with that in the account.

      struct Data {
                  address deprecated_originKey;
            mapping(bytes32 => bool) nonces; // Mapping of nonces
            mapping(address => bool) operationKeys;
            mapping(address => bool) recoveryKeys;
            mapping(address => bool) sudoKeys;
            uint16 sudoKeysCounter;
        }
      
    • If a major release is needed and all account data should be reset and re-initialized, the getStorage() pointer could be changed to use new isolated storage from scratch.

      function getStorage() internal pure returns (Data storage data) {
          bytes32 s = keccak256(abi.encode("io.infinex.Withdraw.v2.0"));
          assembly {
              data.slot := s
          }
      }
      
    • Upgrading the EIP712 version parameter to describe correctly the contract version. This would invalidate previously generated signatures since the domain separator will be different.

    Response by Infinex

    We'll take this into account when performing Account upgrades.

    I-3

    Operation keys can not perform allowlisted withdrawals

    Topic
    Specification
    Impact
    Informational

    The specification describes operational keys to be allowed to execute allowlisted withdrawals, through withdrawERC20ToAllowlistedAddress(), withdrawERC721ToAllowlistedAddress(), and withdrawEtherToAllowlistedAddress().

    However, none of these functions can be called by operation keys since they use the requiresSudoKeySender modifier. Furthermore, this makes the allow listing process redundant as the same access control rank can skip this step and directly execute withdrawals.

    Disclaimer

    Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

    Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

    The scope of this report and review is limited to a review of only the code presented by the Infinex team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.