Security Audit
May 10, 2024
Version 1.0.0
Presented by 0xMacro
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 (*) (**):
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.
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.
Our understanding of the specification was based on the following sources:
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.
Protocol DAO that has control over contract implementation upgrades and configurations as the Owner
role:
trustedRecoveryKeepers
that sets or unsets an address as a trusted recovery keeper who can recover assets from accounts to the owner's recovery address if set.revenuePool
that sets the pool address where fees are sent to.USDC
address of the USDC token contract. (Update of this in the account is Opt-in by Account control).bridgeConfiguration.minimumUSDCBridgeAmount
minimum USDC amount that can be bridged. (Update of this in the account is Opt-in by Account control).Users control their accounts with the following 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.
Multiple Operation Keys can be added by any Sudo key and they can only execute intra-account bridging within different chains.
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.
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.
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.
The following source code was reviewed during the audit:
d267ef612bede48a5ce59a57e31e2ccf416012a8
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 |
|
./src/accounts/AccountFactory.sol |
|
./src/accounts/modules/AccountUtilsModule.sol |
|
./src/accounts/modules/BaseModule.sol |
|
./src/accounts/modules/BridgingModule.sol |
|
./src/accounts/modules/RecoveryModule.sol |
|
./src/accounts/modules/WithdrawModule.sol |
|
./src/accounts/storage/Account.sol |
|
./src/accounts/storage/Bridge.sol |
|
./src/accounts/storage/EIP712.sol |
|
./src/accounts/storage/Recovery.sol |
|
./src/accounts/storage/SecurityKeys.sol |
|
./src/accounts/storage/Withdrawal.sol |
|
./src/accounts/utils/AccountConstants.sol |
|
./src/accounts/utils/RequestTypes.sol |
|
./src/accounts/utils/SecurityModifiers.sol |
|
./src/accounts/utils/WithdrawUtil.sol |
|
./src/beacons/InfinexProtocolConfigBeacon.sol |
|
./src/forwarder/ERC2771Context.sol |
|
./src/forwarder/InfinexERC2771Forwarder.sol |
|
./src/integrations/BridgeIntegrations.sol |
|
./src/interfaces/accounts/IAccountFactory.sol |
|
./src/interfaces/accounts/IAccountUtilsModule.sol |
|
./src/interfaces/accounts/IBaseModule.sol |
|
./src/interfaces/accounts/IBridgingModule.sol |
|
./src/interfaces/accounts/IRecoveryModule.sol |
|
./src/interfaces/accounts/IWithdrawModule.sol |
|
./src/interfaces/beacons/IInfinexProtocolConfigBeacon.sol |
|
./src/libraries/DecimalScaling.sol |
|
./src/libraries/Error.sol |
|
./src/libraries/SolanaUtils.sol |
|
./src/proxy/InfinexProxy.sol |
|
./src/proxy/InitialProxyImplementation.sol |
|
Click on an issue to jump to it, or scroll down to see them all.
AccountFactory
upgrades could lock or steal funds sent to un-deployed accounts
_computeScaledAmount()
takes arbitrary tokens but checks USDC
bridge max amount
We quantify issues in three parts:
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. |
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.
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.
AccountFactory
upgrades could lock or steal funds sent to un-deployed accounts
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.
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.
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.
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.
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.
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()
.
_computeScaledAmount()
takes arbitrary tokens but checks USDC
bridge max amount
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.
_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.
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.
We have plans to use the
_withdraw()
function for other purposes in the future.
recoverToken()
.requiresTrustedRecoveryKey
modifier.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.
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()
.
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.
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
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:
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.
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.
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.
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.
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
}
struct Data {
address originKey;
mapping(bytes32 => bool) nonces; // Mapping of nonces
mapping(address => bool) recoveryKeys;
}
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.
We'll take this into account when performing Account upgrades.
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.
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.