Connext Amarok contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/20f86d58444d7c8178735ada7e456a3112116e54/contracts/contracts/core/connext/facets/PortalFacet.sol#L108

Vulnerability details

Impact

Integer overflow can affect router balances.

Proof of Concept

The repayAavePortal method of the PortalFacet contract subtracts the balance within an unchecked region, but this balance is not checked beforehand to be greater than the amountIn.

unchecked { s.routerBalances[msg.sender][_local] -= amountIn; }

The unique check against the balance is with the _maxIn that always could be greater than the balance because apparently is use as slippage (is not documented as a @param in the methods).

if (routerBalance < _maxIn) revert PortalFacet__repayAavePortal_insufficientFunds();

Recommended Mitigation Steps

Remove the unchecked region.

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L103 https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L134 https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L426

Vulnerability details

Impact

Some contracts assumes decimals <= 18 and does not handle > 18 decimals.

Proof of Concept

Because the pragma used doesn't allow integer underflows, if a token with more than 18 decimals is used, an integer underflow will produce a denial of service.

Some tokens have more than 18 decimals (e.g. YAM-V2 has 24).

This may trigger unexpected reverts due to overflow, posing a liveness risk to the contract.

Reference:

Major severity finding from Consensys Diligence Audit of Defi Saver:

• https://consensys.net/diligence/audits/2021/03/defi-saver/#tokens-with-more-than-18-decimal-points-will-cause-issues

Affected source code:

• https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L103
• https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L134
• https://github.com/code-423n4/2022-06-connext/blob/07adce8e88d3c93e26d32c9c2056593c62911197/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L426

Recommended Mitigation Steps

Ascertain that the code will not fail if the token's decimals are greater than 18.

Low

Front-running init

To initialize contract parameters, most contracts employ an init pattern (rather than a constructor). They are vulnerable to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attack unless they are enforced to be atomic with contact deployment via deployment script or factory contracts.

Many init routines (such as DiamondInit.init) lack an explicit event emission, making it difficult to monitor such scenarios.

Affected source code:

• DiamondInit.sol#L36

Ownable / Pausable

The contract OwnerPausableUpgradeable is Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while paused should be avoided.

Affected source code:

• OwnerPausableUpgradeable.sol#L13

Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

If the owner calls the following methods, it does not verify that the proposed address is valid.

Affected source code:

• ProposedOwnable.sol#L123
• BridgeToken.sol#L202
• ConnextPriceOracle.sol#L168

Outdated compiler

The pragma version used are:

pragma solidity >=0.6.11;
pragma solidity 0.8.14;
pragma solidity >=0.7.6;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

Outdated packages

The packages used are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "4.5.0",
"@openzeppelin/contracts-upgradeable": "4.5.2",

Lack of checks

Check for address(0) during constructor, otherwise it could lead to bad initialization, bad implementations, or bad admin changes.

Affected source code without checking address(0):

• DiamondInit.sol#L42
• PromiseRouter.sol#L146
• PromiseRouter.sol#L155
• RelayerFeeRouter.sol#L81
• RelayerFeeRouter.sol#L90
• XAppConnectionClient.sol#L28
• XAppConnectionClient.sol#L40
• NomadFacet.sol#L26
• LibDiamond.sol#L54
• ConnextPriceOracle.sol#L77
• Executor.sol#L48
• SponsorVault.sol#L127

Wrong comments

Router who took out the credit, and router is not used:

• PortalFacet.sol#L126

Asserts caller is the router owner (if set) or the router itself, the router is only checked if there are no owner:

• RoutersFacet.sol#L156

Todo's left in code

TODO usually mean something still have to be checked of done. This could lead to vulnerabilities if not verified.

Affected source code:

• BridgeFacet.sol#L492
• BridgeFacet.sol#L579

And this seems more important, because routers can repay any-amount out-of-band using the repayAavePortal method or by interacting with the aave contracts directly:

• BridgeFacet.sol#L594-L595

Non Critical

Use encode instead of encodePacked for hashig

Use of abi.encodePacked in ConnextMessage is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

keccak256(abi.encodePacked(bytes(_name).length, _name, bytes(_symbol).length, _symbol, _decimals));:

• ConnextMessage.sol#L178

Double emits

The methods pause and unpause of ProposedOwnableFacet.sol#L253-L261 contract does not check that the contract is already paused or not, it could produce double emits and dApps could be affected.

Codding style

• The file OZERC20.sol contains a contract with another name, ERC20. This is a terrible practice that reduces the readability and auditability of the code.

Delete optimization

Use delete instead of set to default value (false or 0)

Affected source code:

• RoutersFacet.sol#L301
• RoutersFacet.sol#L311
• RoutersFacet.sol#L319
• RoutersFacet.sol#L323
• RoutersFacet.sol#L324
• RoutersFacet.sol#L379
• RoutersFacet.sol#L445-L447
• PromiseRouter.sol#L251
• LibDiamond.sol#L88

Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• LPToken.sol#L21-L25

Logic improve

It's possible to save gas changine the following logic.

Reduce script size changing:

• From AssetLogic.sol#L171-L178 to:

if (_amount == 0 || local == _asset) {
  return (_amount, local);
}

• From AssetLogic.sol#L204-L212 to:

address adopted = s.canonicalToAdopted[id];
if (adopted == _asset || _amount == 0) {
  return (_amount, adopted);
}

Reduce math operations

Use scientific notation (e.g. 10E18) rather than exponentiation (e.g. 10**18)

10**6:

• AmplificationUtils.sol#L22

Store in a constant 10**uint256(POOL_PRECISION_DECIMALS):

• SwapUtils.sol#L370

Dead code

The following contracts are never used. It's an excellent idea to take them out to save gas and improve the codding style.

owner never can be address(0), renounceOwnership require a valid proposed owner:

• ProposedOwnable.sol#L99-L101

The method ERC20._setupDecimals is not used, is initialized like in BridgeToken.sol#L101

Avoid multiple casting

Change the type of the argument _assetId to IERC20 _assetId:

• AssetLogic.sol#L107

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next (require pragma upgrade).

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Affected source code:

• LibDiamond.sol#L66
• LibDiamond.sol#L121
• LibDiamond.sol#L123
• LibDiamond.sol#L132
• LibDiamond.sol#L139
• LibDiamond.sol#L141
• LibDiamond.sol#L150
• LibDiamond.sol#L158
• LibDiamond.sol#L161
• LibDiamond.sol#L170
• LibDiamond.sol#L191
• LibDiamond.sol#L193
• LibDiamond.sol#L224-L228
• SwapUtils.sol#L595
• SwapUtils.sol#L697
• SwapUtils.sol#L784
• OZERC20.sol#L100
• OZERC20.sol#L140
• OZERC20.sol#L185-L190
• OZERC20.sol#L226
• OZERC20.sol#L230
• OZERC20.sol#L254

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• Multicall.sol#L16
• Encoding.sol#L22
• Encoding.sol#L36
• Encoding.sol#L45
• DiamondLoupeFacet.sol#L31
• RelayerFacet.sol#L144
• RelayerFacet.sol#L168
• LibDiamond.sol#L104
• LibDiamond.sol#L129
• LibDiamond.sol#L147
• LibDiamond.sol#L162
• SwapUtils.sol#L205
• SwapUtils.sol#L254
• SwapUtils.sol#L268
• SwapUtils.sol#L289
• SwapUtils.sol#L300
• SwapUtils.sol#L344
• SwapUtils.sol#L405
• SwapUtils.sol#L425
• SwapUtils.sol#L558
• SwapUtils.sol#L591
• SwapUtils.sol#L844
• SwapUtils.sol#L869
• SwapUtils.sol#L924
• SwapUtils.sol#L1014
• SwapUtils.sol#L1019
• SwapUtils.sol#L1039
• SwapUtils.sol#L1055
• ConnextPriceOracle.sol#L176
• StableSwap.sol#L81
• StableSwapFacet.sol#L415
• BridgeFacet.sol#L613
• BridgeFacet.sol#L684
• BridgeFacet.sol#L799

Use the right type

Use bool or uint8 instead of uint256, this could help to save one storage slots:

• LibConnextStorage.sol#L259
• BaseConnextFacet.sol#L13-L15

Optimize struct layout

The following structs could be optimized moving the position of certains values in order to save slot storages:

LibConnextStorage.sol#L38-L52

struct CallParams {
  address to;
  bytes callData;
  uint32 originDomain;
  uint32 destinationDomain;
  address agent;
  address recovery;
  address callback;
  uint256 callbackFee;
  uint256 relayerFee;
  bool forceSlow;       // Move together to destinationDomain
  bool receiveLocal;    // Move together to destinationDomain
  uint256 slippageTol;
}

Move the bool types close to address in LibConnextStorage.sol#L247