QA Report

[code423n4]

QA Report

Table of Contents

• summary
• Comment Missing function parameter
• Events indexing
• Event should be emitted in setters
• Events emitted early
• Function missing comments
• open TODOs
• Payable function not rejecting payments to ERC20 Tokens
• Public functions can be external
• Use scientific notation rather than exponentiation
• Immutable addresses lack zero-address check
• safeApprove is deprecated
• Setters should check the input value
• Timelock in fee setters

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the setters not checking the input value properly and the use of deprecated functions.

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AssetFacet.sol

address _stableSwapPool
ConnextMessage.TokenId calldata _canonical, address _stableSwapPool

BaseConnextFacet.sol

address _potentialReplica

BridgeFacet.sol

XCallArgs calldata _args,address _asset,bytes32 _transferId,uint256 _amount
ExecuteArgs calldata _args
XCallArgs calldata _args, ConnextMessage.TokenId memory _canonical
ExecuteArgs calldata _args
CallParams calldata _params,uint256 _amount,uint256 _nonce,bytes32 _canonicalId,uint32 _canonicalDomain,address _originSender
bytes32 _transferId,bool _isFast,ExecuteArgs calldata _args
ExecuteArgs calldata _args,uint256 _amount,address _asset, // adopted (or local if specified)bytes32 _transferId,bool _reconciled
bytes32 _transferId,uint256 _fastTransferAmount,address _local,address _router
bytes memory _message

PortalFacet.sol

bytes32 _transferId

StableSwapFacet.sol

uint256 minAmountOut,uint256 deadline
uint256 maxAmountIn,uint256 deadline

LPToken.sol

address from,address to,uint256 amount

ProposedOwnableUpgradeable.sol

address newlyProposed

AssetLogic.sol

uint256 _maxIn
uint256 _slippageTol
uint256 _maxIn

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Events indexing

PROBLEM

Events should use indexed fields when possible, up to three per event.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AssetFacet.sol

event WrapperUpdated(address oldWrapper, address newWrapper, address caller)
event TokenRegistryUpdated(address oldTokenRegistry, address newTokenRegistry, address caller)
event StableSwapAdded(bytes32 canonicalId, uint32 domain, address swapPool, address caller)
event AssetAdded(bytes32 canonicalId, uint32 domain, address adoptedAsset, address supportedAsset, address caller)
event AssetRemoved(bytes32 canonicalId, address caller)

BridgeFacet.sol

event XCalled
event Reconciled
event Executed
event TransferRelayerFeesUpdated
event ForcedReceiveLocal
event AavePortalMintUnbacked
event AavePortalRepayment
event AavePortalRepaymentDebt
event SponsorVaultUpdated
event PromiseRouterUpdated
event ExecutorUpdated

PortalFacet.sol

event AavePortalRouterRepayment

ProposedOwnableFacet.sol

event RouterOwnershipRenunciationProposed(uint256 timestamp)
event RouterOwnershipRenounced(bool renounced)
event AssetOwnershipRenunciationProposed(uint256 timestamp)
event AssetOwnershipRenounced(bool renounced)

RelayerFacet.sol

event RelayerFeeRouterUpdated
event RelayerAdded
event RelayerRemoved
event InitiatedClaim
event Claimed

RoutersFacet.sol

event RouterAdded
event RouterRemoved
event MaxRoutersPerTransferUpdated
event LiquidityFeeNumeratorUpdated
event RouterApprovedForPortal
event RouterUnapprovedForPortal
event RouterLiquidityAdded
event RouterLiquidityRemoved

ConnextPriceOracle.sol

event NewAdmin(address oldAdmin, address newAdmin)
event PriceRecordUpdated(address token, address baseToken, address lpToken, bool _active)
event DirectPriceUpdated(address token, uint256 oldPrice, uint256 newPrice)
event AggregatorUpdated(address tokenAddress, address source)
event V1PriceOracleUpdated(address oldAddress, address newAddress)

ProposedOwnableUpgradeable.sol

event RouterOwnershipRenunciationProposed(uint256 timestamp)
event RouterOwnershipRenounced(bool renounced)
event AssetOwnershipRenunciationProposed(uint256 timestamp)
event AssetOwnershipRenounced(bool renounced)
event OwnershipProposed(address indexed proposedOwner)
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner)

SponsorVault.sol

event ConnextUpdated
event RateUpdated
event RelayerFeeCapUpdated
event GasTokenOracleUpdated
event TokenExchangeUpdated
event ReimburseLiquidityFees
event ReimburseRelayerFees
event Deposit
event Withdraw

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events, so that there is up to three indexed fields when possible.

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AssetFacet.sol

emit AssetAdded(_canonical.id, _canonical.domain, _adoptedAssetId, supported, msg.sender) emitted before call to _addStableSwapPool()

LibDiamond.sol

emit DiamondCut(_diamondCut, _init, _calldata) emitted before call to initializeDiamondCut()

TOOLS USED

Manual Analysis

MITIGATION

Place the event emission in the last position in the function.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PortalFacet.sol

setAavePool(address _aavePool)
setAavePortalFee(uint256 _aavePortalFeeNumerator)

StableSwapFacet.sol

setSwapAdminFee(bytes32 canonicalId, uint256 newAdminFee)
setSwapFee(bytes32 canonicalId, uint256 newSwapFee)

SponsorVault.sol

setConnext(address _connext)\

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in all setters.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

AssetFacet.sol

All the getters

BridgeFacet.sol

All the getters and admin methods.

DiamondCutFacet.sol

proposeDiamondCut
rescindDiamondCut

NomadFacet.sol

All the getters

PortalFacet.sol

All the getters

ProposedOwnableFacet.sol

All the internal functions

RelayerFacet.sol

All the getters

RoutersFacet.sol

Some of the getters

ConnextPriceOracle.sol

All the functions

ProposedOwnableUpgradeable.sol

All the internal functions

SponsorVault.sol

_setConnext

LibDiamond.sol

Almost all functions

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

open TODOs

PROBLEM

There are open TODOs in the code. Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BridgeFacet.sol

here
here
here

Executor.sol

here

TOOLS USED

Manual Analysis

MITIGATION

Remove the TODOs

Payable function not rejecting payments to ERC20 Tokens

PROBLEM

In the case of an ERC20 token, functions should check msg.value is zero.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Executor.sol

The execute() function does not check whether msg.value is zero when the token transferred is not ether. Any ETH sent in this situation would get locked as there is no withdrawal function. This is non-critical since the function has the onlyConnext modifier.

TOOLS USED

Manual Analysis

MITIGATION

Check hat msg.value == 0 when !isNative == true

Public functions can be external

PROBLEM

Public functions that are not called by the contract should be declared external.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NotionalTradeModule.sol

redeemMaturedPositions(ISetToken _setToken)

AssetFacet.sol

canonicalToAdopted()
adoptedToCanonical()
approvedAssets()
adoptedToLocalPools()
wrapper()
tokenRegistry()

BridgeFacet.sol

executor()
nonce()
sponsorVault()
promiseRouter()

NomadFacet.sol

remotes()

ProposedOwnableFacet.sol

routerOwnershipRenounced()
assetOwnershipRenounced()
proposed()
proposedTimestamp()
routerOwnershipTimestamp()
assetOwnershipTimestamp()
delay()
proposeRouterOwnershipRenunciation()
renounceRouterOwnership()
proposeAssetOwnershipRenunciation()
renounceAssetOwnership()
renounced()
proposeNewOwner(address newlyProposed)
renounceOwnership()
acceptProposedOwner()
pause()
unpause()

RelayerFacet.sol

transferRelayer(bytes32 _transferId)
approvedRelayers(address _relayer)

RoutersFacet.sol

getProposedRouterOwner
getProposedRouterOwnerTimestamp
maxRoutersPerTransfer
routerBalances
getRouterApprovalForPortal

VersionFacet.sol

VERSION

TOOLS USED

Manual Analysis

MITIGATION

Make these functions external

Use scientific notation rather than exponentiation

PROBLEM

Use scientific notation rather than exponentiation, e.g: 10e10 instead of 10**10

SEVERITY

Non-critical

PROOF OF CONCEPT

Instances include:

SwapUtils.sol

uint256 internal constant MAX_SWAP_FEE = 10**8
uint256 internal constant MAX_ADMIN_FEE = 10**10
uint256 internal constant MAX_LOOP_LIMIT = 256

TOOLS USED

Manual Analysis

MITIGATION

Replace ** with e

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Executor.sol

connext = _connext

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for the function argument.

safeApprove is deprecated

PROBLEM

This function is deprecated.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

AssetLogic.sol

SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn)

TOOLS USED

Manual Analysis

MITIGATION

safeIncreaseAllowance() and safeDecreaseAllowance() should be used instead.

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address or zero

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

NomadFacet.sol

setXAppConnectionManager(address _xAppConnectionManager)

PortalFacet.sol

setAavePool(address _aavePool)

ConnextPriceOracle.sol

setV1PriceOracle(address _v1PriceOracle)
setAdmin(address newAdmin)

SponsorVault.sol

setGasTokenOracle(address _gasTokenOracle)

TOOLS USED

Manual Analysis

MITIGATION

Add zero address checks to these setters

Timelock in fee setters

PROBLEM

Setters that change the fees should use a timelock to give time for users to react. Even if it does not directly impact users, the admin fee can be set as high as a 100%, impacting the earning of liquidity pools. Adding a timelock would give the protocol more legitimacy.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

SwapUtils.sol

setAdminFee
setSwapFee

TOOLS USED

Manual Analysis

MITIGATION

Add a timelock to these setters

[jakekidd]

(-)

• Payable function not rejecting payments to ERC20 Tokens: examples?? it's totally valid for msg.value to != 0 for xcall among others
• Setters should check the input value: valid to set to address(0)
• Timelock in fee setters: time delay feature should come from dao implementation (governor = owner) in the future

(+)

• Comment Missing function parameter: this is great roundup, ty!!