Security Audit
July 3rd, 2024
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for Connext's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team between June 11th and June 14th, 2024.
The purpose of this audit is to review the source code of certain Connext 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 |
Medium | 2 | - | 2 | - |
Low | 1 | - | - | 1 |
Code Quality | 3 | - | - | 3 |
Informational | 2 | - | - | 2 |
Connext was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
98d7fee5777686e8064cd06c31d30f970673a300
94ca7b449493d320293e195da175210a4b8e620a
Specifically, we audited the following contract within this repository:
Contract | SHA256 |
---|---|
src/L1ArbitrumEnabledXERC20.sol |
|
src/L1LockboxGateway.sol |
|
src/L1XERC20Adapter.sol |
|
src/L1XERC20Gateway.sol |
|
src/L2ArbitrumEnabledXERC20.sol |
|
src/L2LockboxGateway.sol |
|
src/L2XERC20Gateway.sol |
|
src/XERC20BaseAdapter.sol |
|
src/XERC20BaseGateway.sol |
|
src/libraries/L1ArbitrumEnabled.sol |
|
src/vendor/L2ArbitrumGateway.sol |
|
src/vendor/L2CustomGateway.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
L1XERC20Adapter
and L1ArbitrumEnabledXERC20
contracts
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 L1XERC20Gateway
contract allows permissionless registration of multiple token pairs meant to be supported for bridging. L1XERC20Gateway
instance is allowed to mint and burn XERC20 tokens according to preconfigured limits. L1XERC20Gateway
is configured with a call to registerTokenOnL2
on an instance of the L1ArbitrumEnabledXERC20
contract if XERC20 is just to be created or on an instance of the L1XERC20Adapter
contract if the XERC20 token already exists. L1XERC20Adapter
, even though a separate contract with a different contract address acts as a stand-in for the XERC20 token. L1XERC20Gateway
, when interacting with the instance of L1XERC20Adapter
, will retrieve a reference to the underlying XERC20 token and perform corresponding operations (mint/burn) directly with the XERC20 token.
A malicious attacker may exploit the current characteristics of the system to mint an unbacked amount of a particular XERC20 token up to the limit allowed for a particular gateway instance. To achieve this attacker may perform the following steps:
L1XERC20Gateway
is configured (registerTokenOnL2
was called on A):
L1ArbitrumEnabledXERC20
or L1XERC20Adapter
L2ArbitrumEnabledXERC20
Attacker creates 2 contracts:
L1XERC20Adapter
for the same XERC20 used in contract A.L2ArbitrumEnabledXERC20
and under full control of the attacker.The attacker registers the C/D token pair with the same L1XERC20Gateway
that has the A/B token pair already registered.
registerTokenFromL1()
is processed on the instance of L2XERC20Gateway
, and l1ToL2Token
mapping is updated to contain a C/D value pair.Attacker mints/obtains an amount of token D up to the specified limit of L1XERC20Gateway
for minting XERC20 referenced in token A.
The attacker invokes L2XERC20Gateway:outboundTransfer()
for token C, resulting in the provided amount of token D being burned, and withdrawal is triggered on L1.
On L1 in L1ArbitrumGateway:finalizeInboundTransfer()
, withdrawal is handled by the L1XERC20Gateway
instance, and L1XERC20Gateway:inboundEscrowTransfer()
is called for C token. Since C is an adapter, getXERC20()
on C will be called, returning the same XERC20 token as the one used in token A. Since there are no additional checks, the attacker will receive a specified amount of valuable XERC20 tokens without providing any backing on L2.
function _inboundEscrowTransfer(address _tokenOrAdapter, address _dest, uint256 _amount) internal virtual {
address _token = _tokenOrAdapter;
if (addressIsAdapter(_tokenOrAdapter)) {
_token = IXERC20Adapter(_tokenOrAdapter).getXERC20();
}
XERC20(_token).mint(_dest, _amount);
}
In summary, the attacker on L2 burns worthless D tokens to obtain a claim for C tokens on L1. However, on L1, a claim for C tokens is incorrectly accepted as a valid claim for valuable A tokens, allowing the attacker to steal A tokens.
Also, there exists an alternative exploit scenario
The attacker registers the C/D token pair on the L1XERC20Gateway
gateway. The attacker on L1 mints worthless C tokens; it bridges them to L2 and obtains a claim to corresponding D tokens. However, since the D token contract is malicious and acts as an IXERC20Adapter
, it returns XERC20 corresponding to a B token on the same gateway. This allows the attacker to mint valuable B tokens with worthless C tokens.
Remediations to consider
L1XERC20Gateway
.Additional changes present in commit https://github.com/BootNodeDev/arbitrum-bridge-erc7281-gateway/pull/17/commits/3fa4e4cd440554f46d7b4c87feb142e707e01e6e
L1XERC20Gateway
inherits from the L1CustomGateway
contract. The forceRegisterTokenToL2()
function implementation provided by the L1CustomGateway
is accessible only by the gateway owner.
This function updates l1ToL2Token
mapping on L1 and L2 through the corresponding counterparty gateway. Since this function does not perform thorough input validations, the caller/owner is responsible for providing correct input.
If the caller/owner is compromised or performs an incorrect update using forceRegisterTokenToL2()
, the bridge may stop working correctly. For example, it may set l2Token in the l1ToL2Token
mapping to a non-expected contract address.
In addition, the L1XERC20Gateway
contract owner, through the use of forceRegisterTokenToL2()
, may register wrapper token (IXERC20Adapter
) for existing tokens and create an alternative path for minting underlying tokens. This would allow the contract owner to perform the same actions as the attacker in the [C-1] issue.
Remediations to consider
forceRegisterTokenToL2()
unless needed.Won't do. Multi-sig or governance are meant to be used as the owner of the L1XERC20Gateway.
L1XERC20Gateway is meant for being managed by a trusted entity, like the Arbitrum DAO, so the forceRegisterTokenToL2 is there as a safety net. If someone other than a token issuer miss-registered a token using an adapter, the forceRegisterTokenToL2 could be used for fixing the registration.
For successful bridge operations, which include XERC20 tokens implemented by third parties, it is important that configured mint and burn limits for instances of custom gateways (L1XERC20Gateway
, L1LockboxGateway
, L2XERC20Gateway
, L2LockboxGateway
) are appropriate and flexible so as not to prevent normal bridge operation.
Compromised or malicious XERC20 owner may change limits through XERC20:setLimits()
and cause all bridge operations in previously mentioned gateway contracts to fail whenever one of these two functions is part of their execution trace:
XERC20BaseGateway:_outboundEscrowTransfer()
XERC20BaseGateway:_inboundEscrowTransfer()
As a result, bridging from L1 to L2 and vice versa, as well as unwrapping from the XERC20 to the base token, may not work, resulting in DoS.
Remediations to consider:
Won't do. Multi-sig or governance will be used as the owner of the L1XERC20Gateway.
L1XERC20Adapter
and L1ArbitrumEnabledXERC20
contracts
L1XERC20Adapter.registerTokenOnL2()
and L1ArbitrumEnabledXERC20.registerTokenOnL2()
functions accept native token in order to send to the gateway and router appropriate amounts defined by valueForGateway
and valueForRouter
, respectively:
// L1ArbitrumEnabled
function _registerTokenOnL2(
address l2TokenAddress,
uint256 maxSubmissionCostForGateway,
uint256 maxSubmissionCostForRouter,
uint256 maxGasForGateway,
uint256 maxGasForRouter,
uint256 gasPriceBid,
uint256 valueForGateway,
uint256 valueForRouter,
address creditBackAddress
)
internal
{
IL1CustomGateway gateway = IL1CustomGateway(gatewayAddress);
gateway.registerTokenToL2{ value: valueForGateway }(
l2TokenAddress, maxGasForGateway, gasPriceBid, maxSubmissionCostForGateway, creditBackAddress
);
IL1GatewayRouter(gateway.router()).setGateway{ value: valueForRouter }(
gatewayAddress, maxGasForRouter, gasPriceBid, maxSubmissionCostForRouter, creditBackAddress
);
}
However, if the native token amount provided exceeds the sum of valueForGateway
and valueForRouter
amounts, it will remain stuck in the contract, as there is no way to retrieve the excess amount.
Remediations to Consider
Add validation to check that the function’s msg.value
is correct
function registerTokenOnL2(...)
{
+ require(msg.value == valueForGateway + valueForGateway, "WRONG_VALUE");
_registerTokenOnL2(
l2TokenAddress,
maxSubmissionCostForGateway,
maxSubmissionCostForRouter,
maxGasForGateway,
maxGasForRouter,
gasPriceBid,
valueForGateway,
valueForGateway,
creditBackAddress
);
}
L1XERC20Adapter
contract specification in docs has the following:
Implements all the read-only functions of an XERC20 token which forwards the calls to the actual token related to this adapter, this would help a smoother integration with UI
However, the XERC20BaseAdapter
contract does expose ALL of XERC20
’s view functions. These are all the functions that the contract missed:
XERC20
's:mintingMaxLimitOf()
burningMaxLimitOf()
mintingCurrentLimitOf()
burningCurrentLimitOf()
FACTORY()
lockbox()
bridges()
ERC20Permit
's:nonces()
DOMAIN_SEPARATOR()
EIP712
's:eip712Domain()
Ownable
's:owner()
Remediations to consider
Addressed by updating the associated specification document.
Natspec for L2XERC20Gateway.finalizeInboundTransfer()
indicates that if the corresponding L2 token is not yet deployed, it will deploy it. However, in the L2XERC20Gateway
implementation, this scenario is handled by performing a withdrawal back to L1. Consider updating natspec to describe this specific behavior correctly.
* @notice Mint on L2 upon L1 deposit.
=> * If token not yet deployed and symbol/name/decimal data is included, deploys StandardArbERC20
* @dev Callable only by the L1ERC20Gateway.outboundTransfer method. For initial deployments of a token the L1
* L1ERC20Gateway
=> * is expected to include the deployData. If not a L1 withdrawal is automatically triggered for the user
XERC20BaseAdapter contract features Ownable OZ functionality. However, this functionality is never used.
Consider removing it unless needed for specific use case.
Fee on transfer and rebase tokens are not supported by the current implementation. Therefore, relevant actors should not register tokens with these incompatible behavior patterns.
Wont do. This solution is not meant to support those kind of tokens. We may address this in future update.
In the L1XERC20Gateway
, if mint/burn limits are set for a particular XERC20 token before it is registered, a malicious attacker may front-run token registration by registering an IXERC20Adapter wrapper for the same token. With successful wrapper token registration, the attacker may then mint the XERC20 token up to the approved limit for L1XERC20Gateway.
Due to the above, the XERC20 registration procedure and the order of steps performed should be carefully considered.
We have updated documentation to indicate additional security requirements.
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 Connext 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.