Connext Amarok contest - oyc_109's results

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);

> 0 is less efficient than != 0 for unsigned integers (with proof)

description

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs.

https://twitter.com/gzeon/status/1485428085885640706

findings

/2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol
499: if (_amount > 0) {
665: if (pathLength > 0)

use calldata instead of memory

description

Use calldata instead of memory for function parameters saves gas if the function argument is only read.

findings

/2022-06-connext/contracts/contracts/core/connext/facets/BridgeFacet.sol
541: function _reconcile(uint32 _origin, bytes memory _message) internal {
706: function _getTransferId(XCallArgs calldata _args, ConnextMessage.TokenId memory _canonical)

/2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol
395:     IERC20[] memory _pooledTokens,
396:     uint8[] memory decimals,
397:     string memory lpTokenName,
398:     string memory lpTokenSymbol,

/2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol
62:     IERC20[] memory _pooledTokens,
63:     uint8[] memory decimals,
64:     string memory lpTokenName,
65:     string memory lpTokenSymbol,

cache in variables instead of loading

description

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

findings

/2022-06-connext/contracts/contracts/core/connext/facets/RelayerFacet.sol
140: for (uint256 i; i < _transferIds.length; ) {
164: for (uint256 i; i < _transferIds.length; ) {

for loop optimisations

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

Suggest not initializing the for loop counter to 0.

An array’s length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Suggest storing the array’s length in a variable before the for-loop, and use it instead:

++i costs less gas compared to i++

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

taking all of the above, the recommended format for gas savings is

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

findings

/2022-06-connext/contracts/contracts/core/connext/facets/StableSwapFacet.sol
415: for (uint8 i = 0; i < _pooledTokens.length; i++) {

/2022-06-connext/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol
176: for (uint256 i = 0; i < tokenAddresses.length; i++) {

/2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol
81: for (uint8 i = 0; i < _pooledTokens.length; i++) {

/2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol
205: for (uint256 i = 0; i < xp.length; i++) {

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

/2022-06-connext/contracts/contracts/core/connext/facets/VersionFacet.sol
16: uint8 internal immutable _version = 0;

use custom errors

description

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

findings

/2022-06-connext/contracts/contracts/core/connext/helpers/StableSwap.sol
75:     require(_pooledTokens.length > 1, "_pooledTokens.length <= 1");
76:     require(_pooledTokens.length <= 32, "_pooledTokens.length > 32");
77:     require(_pooledTokens.length == decimals.length, "_pooledTokens decimals mismatch");

do not calculate constants

description

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

Consequences: each usage of a "constant" costs more gas on each access. Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

findings

/2022-06-connext/contracts/contracts/core/connext/libraries/SwapUtils.sol
104: uint256 internal constant FEE_DENOMINATOR = 10**10;
107: uint256 internal constant MAX_SWAP_FEE = 10**8;
113: uint256 internal constant MAX_ADMIN_FEE = 10**10;