Connext Amarok contest - SmartSek's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L279-L286

Vulnerability details

Impact

In StableSwapFacet.sol, two swapping functions contain the whenNotPaused modifier while swapExactOut() and addSwapLiquidity() do not. All functions to swap and add liquidity should contain the same modifiers to stop transactions while paused.

Proof of Concept

Example with modifier

  function swapExact(
    bytes32 canonicalId,
    uint256 amountIn,
    address assetIn,
    address assetOut,
    uint256 minAmountOut,
    uint256 deadline
  ) external payable nonReentrant deadlineCheck(deadline) whenNotPaused returns (uint256) {

Examples without modifier

  function swapExactOut(
    bytes32 canonicalId,
    uint256 amountOut,
    address assetIn,
    address assetOut,
    uint256 maxAmountIn,
    uint256 deadline
  ) external payable nonReentrant deadlineCheck(deadline) returns (uint256) {

and

  function addSwapLiquidity(
    bytes32 canonicalId,
    uint256[] calldata amounts,
    uint256 minToMint,
    uint256 deadline
  ) external nonReentrant deadlineCheck(deadline) returns (uint256) {
    return s.swapStorages[canonicalId].addLiquidity(amounts, minToMint);
  }

Tools Used

Manual review.

Recommended Mitigation Steps

Add the whenNotPaused modifier to all functions that perform swaps or liquidity additions.

QA Report

[L-01] Missing zero address check

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L48 If parameter is accidentally set to zero the contract will have to be redeployed

---

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L155-L158 Setter function would have to be called again.

[L-02] Inconsistent, outdated and floating compiler versions

Using a floating pragma might result in contracts being deployed with a version they were not tested with and might result in bugs that affect the contracts system negatively. In addition, older compilers might be susceptible to some bugs. A list of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version or a version it was not tested with. We recommend changing the solidity version pragma to the latest version to enforce the use of an up-to-date compiler.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/PriceOracle.sol#L2 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/shared/Router.sol#L2

[L-03] Division before multiplication

Performing a division before a multiplication might lead to precision loss due to truncation.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L303 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L309

[L-04] Uncommented Facet struct

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/DiamondLoupeFacet.sol#L20-L23

  // struct Facet {
  //     address facetAddress;
  //     bytes4[] functionSelectors;
  // }

Facet struct is commented out, even though it is used in the contract.

[L-05] Code vs Comment Conflict: Router can set recipient address multiple times.

Comments state that the recipient address should only be settable by the router once but there are no checks in place.

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L386-L403

[L-06] Use of deprecated safeApprove()

OpenZeppelin lists this function as deprecated. It is recommended to use safeIncreaseAllowance.

OpenZeppelin Documentation: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fcf35e5722847f5eadaaee052968a8a54d03622a/contracts/token/ERC20/utils/SafeERC20.sol#L39-L45

Link to code: https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/AssetLogic.sol#L347

[L-07] Incorrect event emissions when deposit is made with ERC20 and msg.value

If the user calls deposit() and _token != address(0), then the event will be emitted for the ERC20 token. If the user also had mistakenly sent a positive msg.value, then this event will not be emitted.

This function should check both conditions to potentially emit multiple events.

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/SponsorVault.sol#L271-L277

[N-01] Remove TODOs

Please remove TODOs as they harm readability for auditing.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L709 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L1027 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L492 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L7 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L303

[N-02] Remove redundant comment

// return keccak256(abi.encode(s.nonce, _args.params, msg.sender, _canonical.id, _canonical.domain, _args.amount));

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L871

[N-03] deadline input missing @param comment.

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L248-L253

deadline input is missing @param comment as in this example.

Please implement comment for better clarity and consistency.

Gas Report

[G-01] for loop gas optimization

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L215

for (uint256 i = 0; i < tokenAddresses.length; i++) {
    aggregators[tokenAddresses[i]] = AggregatorV3Interface(sources[i]);
    emit AggregatorUpdated(tokenAddresses[i], sources[i]);
}

Gas could be saved by:

• Not initializing variable to default value of zero
• Caching array length
• Using a prefix (++i) instead of a postfix (i++)
• Unchecking increment count

Example:

length = tokenAddresses.length;
for (uint256 i; i < length;) {
    aggregators[tokenAddresses[i]] = AggregatorV3Interface(sources[i]);
    emit AggregatorUpdated(tokenAddresses[i], sources[i]);
		unchecked { ++i; }
}

Other instances: https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L81 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L268 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L1019 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L1055

[G-02] Unnecessary use of SafeMath

The use of SafeMath to prevent underflow/overflow is not necessary if using compiler version ^0.8.0. https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L161