QA Report

[code423n4]

Event is missing indexed fields

description

Each event should use three indexed fields if there are three or more fields

findings

/2022-06-connext/contracts/contracts/core/connext/facets/AssetFacet.sol
27: event WrapperUpdated(address oldWrapper, address newWrapper, address caller);
35: event TokenRegistryUpdated(address oldTokenRegistry, address newTokenRegistry, address caller);
44: event StableSwapAdded(bytes32 canonicalId, uint32 domain, address swapPool, address caller);
55: event AssetAdded(bytes32 canonicalId, uint32 domain, address adoptedAsset, address supportedAsset, address caller);

missing checks for zero address

description

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

findings

/2022-06-connext/contracts/contracts/core/connext/facets/AssetFacet.sol
104: s.wrapper = IWrapped(_wrapper);
117: s.tokenRegistry = ITokenRegistry(_tokenRegistry);

/2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol
238: s.promiseRouter = PromiseRouter(_promiseRouter);
246: s.executor = IExecutor(_executor);
254: s.sponsorVault = ISponsorVault(_sponsorVault);

missing/incomplete NATSPEC

description

/2022-06-connext/contracts/contracts/core/connext/facets/AssetFacet.sol
159:   /**
160:    * @notice Adds a stable swap pool for the local <> adopted asset.
161:    */

lack of time delay period for removing assets

description

the owner can call AssetFacet.removeAssetId to remove any approved assets and pools

recommend adding a time delay to this so that users will not lose funds due to assets being removed

findings

/2022-06-connext/contracts/contracts/core/connext/facets/AssetFacet.sol
171: function removeAssetId(bytes32 _canonicalId, address _adoptedAssetId) external onlyOwner {

The nonReentrant modifier should occur before all other modifiers

description

This is a best-practice to protect against reentrancy in other modifiers

findings

/2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol
279: function xcall(XCallArgs calldata _args) external payable whenNotPaused nonReentrant returns (bytes32) {

open TODOs

description

Open TODOs can hint at programming or architectural errors that still need to be fixed.

Recommend resolving the TODO and bubble up the error.

findings

/2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol
492: // TODO: do we want to store a mapping of custodied token balances here?
579: // TODO: do we need to keep this
1027: // TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?

/2022-06-connext/contracts/contracts/core/connext/helpers/Executor.sol
7: // TODO: see note in below file re: npm

No Transfer Ownership Pattern

description

Recommend considering implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

findings

/2022-06-connext/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol
168:   function setAdmin(address newAdmin) external onlyAdmin {
169:     address oldAdmin = admin;
170:     admin = newAdmin;
171: 
172:     emit NewAdmin(oldAdmin, newAdmin);
173:   }

admin can increase fee with no time delay

description

in StableSwap.sol the admin can increase fees by calling setAdminFee and setSwapFee with no time delay

a malicious admin can front run transactions to jack up the fee

recommend adding a time delay for any fee changes

/2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol
444:   /**
445:    * @notice Update the admin fee. Admin fee takes portion of the swap fee.
446:    * @param newAdminFee new admin fee to be applied on future transactions
447:    */
448:   function setAdminFee(uint256 newAdminFee) external onlyOwner {
449:     swapStorage.setAdminFee(newAdminFee);
450:   }
451: 
452:   /**
453:    * @notice Update the swap fee to be applied on swaps
454:    * @param newSwapFee new swap fee to be applied on future transactions
455:    */
456:   function setSwapFee(uint256 newSwapFee) external onlyOwner {
457:     swapStorage.setSwapFee(newSwapFee);
458:   }

safeApprove() is deprecated

description

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance()

findings

/2022-06-connext/contracts/contracts/core/connext/libraries/AssetLogic.sol
347: SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);

[jakekidd]

2 invalid, we need to be able to set 0 address for some of these
4 sort of invalid, sort of just 'acknowledged' here; time delay feature should come from dao implementation (governor = owner) in the future
6 open TODOs here aren't hinting at errors, but a good note - resolved these
8 same as 4
9 invalid -approval needs to be reset to 0 and then increased, so we are stuck using this method