Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

QA Report #186

Open
code423n4 opened this issue Jun 19, 2022 · 5 comments
Open

QA Report #186

code423n4 opened this issue Jun 19, 2022 · 5 comments
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

Summary

Low Risk Issues

Issue Instances
1 Unused/empty receive()/fallback() function 1
2 safeApprove() is deprecated 1
3 Missing checks for address(0x0) when assigning values to address state variables 8
4 abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() 1
5 Open TODOs 5
6 Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions 2

Total: 18 instances over 6 issues

Non-critical Issues

Issue Instances
1 Missing initializer modifier on constructor 2
2 Unused file 1
3 The nonReentrant modifier should occur before all other modifiers 2
4 Contract implements interface without extending the interface 3
5 public functions not called by the contract should be declared external instead 45
6 Non-assembly method available 1
7 constants should be defined rather than using magic numbers 30
8 Expressions for constant values such as a call to keccak256(), should use immutable rather than constant 1
9 Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18) 4
10 Variable names that consist of all capital letters should be reserved for constant/immutable variables 6
11 Non-library/interface files should use fixed compiler versions, not floating ones 1
12 File is missing NatSpec 2
13 NatSpec is incomplete 36
14 Event is missing indexed fields 69
15 Not using the named return variables anywhere in the function is confusing 2

Total: 205 instances over 15 issues

Low Risk Issues

1. Unused/empty receive()/fallback() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth)))

There is 1 instance of this issue:

File: contracts/contracts/core/promise/PromiseRouter.sol   #1

132:    receive() external payable {}

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L132

2. safeApprove() is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead

There is 1 instance of this issue:

File: contracts/contracts/core/connext/libraries/AssetLogic.sol   #1

347:          SafeERC20.safeApprove(IERC20(_assetIn), address(pool), _amountIn);

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

3. Missing checks for address(0x0) when assigning values to address state variables

There are 8 instances of this issue:

File: contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol

77:       wrapped = _wrapped;

165:      v1PriceOracle = _v1PriceOracle;

170:      admin = newAdmin;

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

File: contracts/contracts/core/connext/helpers/Executor.sol

48:       connext = _connext;

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

File: contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol

315:      _owner = newOwner;

322:      _proposed = newlyProposed;

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

File: contracts/contracts/core/shared/ProposedOwnable.sol

163:      _owner = newOwner;

171:      _proposed = newlyProposed;

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/shared/ProposedOwnable.sol#L163

4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

If all arguments are strings and or bytes, bytes.concat() should be used instead

There is 1 instance of this issue:

File: contracts/contracts/core/connext/libraries/ConnextMessage.sol   #1

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

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

5. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

There are 5 instances of this issue:

File: 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

594:      // FIXME: routers can repay any-amount out-of-band using the `repayAavePortal` method

1027:     // TODO: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch to not affect gas on all cases?

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

File: contracts/contracts/core/connext/libraries/LibConnextStorage.sol

303:    // BridgeFacet (cont.) TODO: can we move this

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

6. Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

There are 2 instances of this issue:

File: contracts/contracts/core/connext/helpers/LPToken.sol   #1

13:   contract LPToken is ERC20BurnableUpgradeable, OwnableUpgradeable {

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

File: contracts/contracts/core/connext/helpers/StableSwap.sol   #2

31:   contract StableSwap is IStableSwap, OwnerPausableUpgradeable, ReentrancyGuardUpgradeable {

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

Non-critical Issues

1. Missing initializer modifier on constructor

OpenZeppelin recommends that the initializer modifier be applied to constructors

There are 2 instances of this issue:

File: contracts/contracts/core/connext/helpers/StableSwap.sol   #1

31:   contract StableSwap is IStableSwap, OwnerPausableUpgradeable, ReentrancyGuardUpgradeable {

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

File: contracts/contracts/core/promise/PromiseRouter.sol   #2

23:   contract PromiseRouter is Version, Router, ReentrancyGuardUpgradeable {

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

2. Unused file

The file is never imported by any other file

There is 1 instance of this issue:

File: contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol   #1

0:    // SPDX-License-Identifier: UNLICENSED

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

3. The nonReentrant modifier should occur before all other modifiers

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

There are 2 instances of this issue:

File: contracts/contracts/core/connext/facets/BridgeFacet.sol   #1

279:    function xcall(XCallArgs calldata _args) external payable whenNotPaused nonReentrant returns (bytes32) {

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

File: contracts/contracts/core/connext/facets/BridgeFacet.sol   #2

411:    function execute(ExecuteArgs calldata _args) external whenNotPaused nonReentrant returns (bytes32) {

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

4. Contract implements interface without extending the interface

Not extending the interface may lead to the wrong function signature being used, leading to unexpected behavior. If the interface is in fact being implemented, use the override keyword to indicate that fact

There are 3 instances of this issue:

File: contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol   #1

/// @audit owner(), renounceOwnership()
28:   contract ProposedOwnableFacet is BaseConnextFacet, IProposedOwnable {

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

File: contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol   #2

/// @audit owner(), renounceOwnership()
26:   abstract contract ProposedOwnableUpgradeable is Initializable {

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

File: contracts/contracts/core/shared/ProposedOwnable.sol   #3

/// @audit owner(), renounceOwnership()
28:   abstract contract ProposedOwnable is IProposedOwnable {

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/shared/ProposedOwnable.sol#L28

5. public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There are 45 instances of this issue:

File: contracts/contracts/core/connext/facets/VersionFacet.sol

20:     function VERSION() public pure returns (uint8) {

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

File: contracts/contracts/core/connext/facets/BridgeFacet.sol

199:    function relayerFees(bytes32 _transferId) public view returns (uint256) {

203:    function routedTransfers(bytes32 _transferId) public view returns (address[] memory) {

207:    function reconciledTransfers(bytes32 _transferId) public view returns (bool) {

211:    function domain() public view returns (uint256) {

215:    function executor() public view returns (IExecutor) {

219:    function nonce() public view returns (uint256) {

223:    function sponsorVault() public view returns (ISponsorVault) {

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

File: contracts/contracts/core/connext/facets/AssetFacet.sol

66:     function canonicalToAdopted(bytes32 _canonicalId) public view returns (address) {

70:     function adoptedToCanonical(address _adopted) public view returns (ConnextMessage.TokenId memory) {

78:     function approvedAssets(bytes32 _asset) public view returns (bool) {

82:     function adoptedToLocalPools(bytes32 _adopted) public view returns (IStableSwap) {

86:     function wrapper() public view returns (IWrapped) {

90:     function tokenRegistry() public view returns (ITokenRegistry) {

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

File: contracts/contracts/core/connext/facets/RelayerFacet.sol

70:     function transferRelayer(bytes32 _transferId) public view returns (address) {

74:     function approvedRelayers(address _relayer) public view returns (bool) {

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

File: contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol

76:     function routerOwnershipRenounced() public view returns (bool) {

83:     function assetOwnershipRenounced() public view returns (bool) {

90:     function proposed() public view returns (address) {

97:     function proposedTimestamp() public view returns (uint256) {

104:    function routerOwnershipTimestamp() public view returns (uint256) {

111:    function assetOwnershipTimestamp() public view returns (uint256) {

118:    function delay() public view returns (uint256) {

128:    function proposeRouterOwnershipRenunciation() public onlyOwner {

142:    function renounceRouterOwnership() public onlyOwner {

162:    function proposeAssetOwnershipRenunciation() public onlyOwner {

175:    function renounceAssetOwnership() public onlyOwner {

195:    function renounced() public view returns (bool) {

203:    function proposeNewOwner(address newlyProposed) public onlyOwner {

217:    function renounceOwnership() public onlyOwner {

236:    function acceptProposedOwner() public onlyProposed {

253:    function pause() public onlyOwner {

258:    function unpause() public onlyOwner {

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

File: contracts/contracts/core/connext/facets/NomadFacet.sol

11:     function xAppConnectionManager() public view returns (XAppConnectionManager) {

15:     function remotes(uint32 _domain) public view returns (bytes32) {

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

File: contracts/contracts/core/connext/facets/RoutersFacet.sol

181:    function LIQUIDITY_FEE_NUMERATOR() public view returns (uint256) {

185:    function LIQUIDITY_FEE_DENOMINATOR() public view returns (uint256) {

222:    function getProposedRouterOwner(address _router) public view returns (address) {

231:    function getProposedRouterOwnerTimestamp(address _router) public view returns (uint256) {

235:    function maxRoutersPerTransfer() public view returns (uint256) {

239:    function routerBalances(address _router, address _asset) public view returns (uint256) {

247:    function getRouterApprovalForPortal(address _router) public view returns (bool) {

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

File: contracts/contracts/core/promise/PromiseRouter.sol

146:    function initialize(address _xAppConnectionManager) public initializer {

230:    function process(bytes32 transferId, bytes calldata _message) public nonReentrant {

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L146

File: contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol

80:     function initialize(address _xAppConnectionManager) public initializer {

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol#L80

6. Non-assembly method available

assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length

There is 1 instance of this issue:

File: contracts/contracts/core/connext/libraries/LibDiamond.sol   #1

245:        contractSize := extcodesize(_contract)

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

7. constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 30 instances of this issue:

File: contracts/contracts/core/connext/facets/StableSwapFacet.sol

/// @audit 32
408:      if (_pooledTokens.length <= 1 || _pooledTokens.length > 32)

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

File: contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol

/// @audit 9995
76:         s.LIQUIDITY_FEE_NUMERATOR = 9995;

/// @audit 10000
77:         s.LIQUIDITY_FEE_DENOMINATOR = 10000;

/// @audit 5
78:         s.maxRoutersPerTransfer = 5;

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol#L76

File: contracts/contracts/core/connext/facets/RoutersFacet.sol

/// @audit 95
349:      if (_numerator < (denominator * 95) / 100) revert RoutersFacet__setLiquidityFeeNumerator_tooSmall();

/// @audit 100
349:      if (_numerator < (denominator * 95) / 100) revert RoutersFacet__setLiquidityFeeNumerator_tooSmall();

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

File: contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol

/// @audit 18
103:        uint256 tokenDecimalDelta = 18 - uint256(IERC20Extended(priceInfo.token).decimals());

/// @audit 18
106:        uint256 baseTokenDecimalDelta = 18 - uint256(IERC20Extended(priceInfo.baseToken).decimals());

/// @audit 18
134:        uint256 price = retVal.mul(10**(18 - uint256(aggregator.decimals())));

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

File: contracts/contracts/core/connext/helpers/StableSwap.sol

/// @audit 32
76:       require(_pooledTokens.length <= 32, "_pooledTokens.length > 32");

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

File: contracts/contracts/core/connext/libraries/SwapUtils.sol

/// @audit 4
626:      return swapFee.mul(numTokens).div(numTokens.sub(1).mul(4));

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

File: contracts/contracts/core/connext/libraries/ConnextMessage.sol

/// @audit 4
232:      return uint32(_tokenId.indexUint(0, 4));

/// @audit 4
242:      return _tokenId.index(4, 32);

/// @audit 32
242:      return _tokenId.index(4, 32);

/// @audit 16
252:      return _tokenId.indexAddress(16);

/// @audit 32
282:      return _transferAction.index(1, 32);

/// @audit 13
292:      return _transferAction.indexAddress(13);

/// @audit 33
302:      return _transferAction.indexUint(33, 32);

/// @audit 32
302:      return _transferAction.indexUint(33, 32);

/// @audit 97
312:      return _transferAction.index(97, 32);

/// @audit 32
312:      return _transferAction.index(97, 32);

/// @audit 65
322:      return _transferAction.index(65, 32);

/// @audit 32
322:      return _transferAction.index(65, 32);

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

File: contracts/contracts/core/connext/libraries/LibCrossDomainProperty.sol

/// @audit 5
120:      return _property.indexAddress(5);

/// @audit 4
130:      return uint32(_property.indexUint(1, 4));

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

File: contracts/contracts/core/promise/PromiseRouter.sol

/// @audit 32
313:      return (uint64(_origin) << 32) | _nonce;

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L313

File: contracts/contracts/core/promise/libraries/PromiseMessage.sol

/// @audit 32
81:       return _view.index(1, 32);

/// @audit 33
91:       return _view.indexAddress(33);

/// @audit 53
101:      return _view.indexUint(53, 1) == 1;

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/libraries/PromiseMessage.sol#L81

File: contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol

/// @audit 32
166:      return (uint64(_origin) << 32) | _nonce;

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol#L166

8. Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

There is 1 instance of this issue:

File: contracts/contracts/core/connext/libraries/LibDiamond.sol   #1

14:     bytes32 constant DIAMOND_STORAGE_POSITION = keccak256("diamond.standard.diamond.storage");

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

9. Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)

There are 4 instances of this issue:

File: contracts/contracts/core/connext/libraries/AmplificationUtils.sol   #1

22:     uint256 public constant MAX_A = 10**6;

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

File: contracts/contracts/core/connext/libraries/SwapUtils.sol   #2

104:    uint256 internal constant FEE_DENOMINATOR = 10**10;

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

File: contracts/contracts/core/connext/libraries/SwapUtils.sol   #3

107:    uint256 internal constant MAX_SWAP_FEE = 10**8;

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

File: contracts/contracts/core/connext/libraries/SwapUtils.sol   #4

113:    uint256 internal constant MAX_ADMIN_FEE = 10**10;

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

10. Variable names that consist of all capital letters should be reserved for constant/immutable variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).

There are 6 instances of this issue:

File: contracts/contracts/core/connext/helpers/Executor.sol

38:     uint256 public FAILURE_GAS = 100_000; // Allowance decrease + transfer

43:     uint16 public MAX_COPY = 256;

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

File: contracts/contracts/core/connext/libraries/LibConnextStorage.sol

115:    uint256 LIQUIDITY_FEE_NUMERATOR;

117:    uint256 LIQUIDITY_FEE_DENOMINATOR;

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

File: contracts/contracts/core/promise/PromiseRouter.sol

64:     uint256[49] private __GAP;

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L64

File: contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol

38:     uint256[49] private __GAP;

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol#L38

11. Non-library/interface files should use fixed compiler versions, not floating ones

There is 1 instance of this issue:

File: contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol   #1

2:    pragma solidity ^0.8.0;

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol#L2

12. File is missing NatSpec

There are 2 instances of this issue:

File: contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol (various lines)   #1

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol

File: contracts/contracts/core/connext/libraries/LibDiamond.sol (various lines)   #2

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

13. NatSpec is incomplete

There are 36 instances of this issue:

File: contracts/contracts/core/connext/facets/StableSwapFacet.sol

/// @audit Missing: '@return'
235      * @param deadline latest timestamp to accept this transaction
236      */
237     function swap(
238       bytes32 canonicalId,
239       uint8 tokenIndexFrom,
240       uint8 tokenIndexTo,
241       uint256 dx,
242       uint256 minDy,
243       uint256 deadline
244:    ) external nonReentrant deadlineCheck(deadline) whenNotPaused returns (uint256) {

/// @audit Missing: '@param minAmountOut'
248     /**
249      * @notice Swap two tokens using this pool
250      * @param canonicalId the canonical token id
251      * @param assetIn the token the user wants to swap from
252      * @param assetOut the token the user wants to swap to
253      * @param amountIn the amount of tokens the user wants to swap from
254      */
255     function swapExact(
256       bytes32 canonicalId,
257       uint256 amountIn,
258       address assetIn,
259       address assetOut,
260       uint256 minAmountOut,
261       uint256 deadline
262:    ) external payable nonReentrant deadlineCheck(deadline) whenNotPaused returns (uint256) {

/// @audit Missing: '@param deadline'
248     /**
249      * @notice Swap two tokens using this pool
250      * @param canonicalId the canonical token id
251      * @param assetIn the token the user wants to swap from
252      * @param assetOut the token the user wants to swap to
253      * @param amountIn the amount of tokens the user wants to swap from
254      */
255     function swapExact(
256       bytes32 canonicalId,
257       uint256 amountIn,
258       address assetIn,
259       address assetOut,
260       uint256 minAmountOut,
261       uint256 deadline
262:    ) external payable nonReentrant deadlineCheck(deadline) whenNotPaused returns (uint256) {

/// @audit Missing: '@return'
248     /**
249      * @notice Swap two tokens using this pool
250      * @param canonicalId the canonical token id
251      * @param assetIn the token the user wants to swap from
252      * @param assetOut the token the user wants to swap to
253      * @param amountIn the amount of tokens the user wants to swap from
254      */
255     function swapExact(
256       bytes32 canonicalId,
257       uint256 amountIn,
258       address assetIn,
259       address assetOut,
260       uint256 minAmountOut,
261       uint256 deadline
262:    ) external payable nonReentrant deadlineCheck(deadline) whenNotPaused returns (uint256) {

/// @audit Missing: '@param maxAmountIn'
272     /**
273      * @notice Swap two tokens using this pool
274      * @param canonicalId the canonical token id
275      * @param assetIn the token the user wants to swap from
276      * @param assetOut the token the user wants to swap to
277      * @param amountOut the amount of tokens the user wants to swap to
278      */
279     function swapExactOut(
280       bytes32 canonicalId,
281       uint256 amountOut,
282       address assetIn,
283       address assetOut,
284       uint256 maxAmountIn,
285       uint256 deadline
286:    ) external payable nonReentrant deadlineCheck(deadline) returns (uint256) {

/// @audit Missing: '@param deadline'
272     /**
273      * @notice Swap two tokens using this pool
274      * @param canonicalId the canonical token id
275      * @param assetIn the token the user wants to swap from
276      * @param assetOut the token the user wants to swap to
277      * @param amountOut the amount of tokens the user wants to swap to
278      */
279     function swapExactOut(
280       bytes32 canonicalId,
281       uint256 amountOut,
282       address assetIn,
283       address assetOut,
284       uint256 maxAmountIn,
285       uint256 deadline
286:    ) external payable nonReentrant deadlineCheck(deadline) returns (uint256) {

/// @audit Missing: '@return'
272     /**
273      * @notice Swap two tokens using this pool
274      * @param canonicalId the canonical token id
275      * @param assetIn the token the user wants to swap from
276      * @param assetOut the token the user wants to swap to
277      * @param amountOut the amount of tokens the user wants to swap to
278      */
279     function swapExactOut(
280       bytes32 canonicalId,
281       uint256 amountOut,
282       address assetIn,
283       address assetOut,
284       uint256 maxAmountIn,
285       uint256 deadline
286:    ) external payable nonReentrant deadlineCheck(deadline) returns (uint256) {

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

File: contracts/contracts/core/connext/facets/BridgeFacet.sol

/// @audit Missing: '@return'
625      * @param _sig The signature you are recovering the signer from
626      */
627:    function _recoverSignature(bytes32 _signed, bytes calldata _sig) internal pure returns (address) {

/// @audit Missing: '@return'
743      * @param _liquidityFeeDen Liquidity fee denominator
744      */
745     function _getFastTransferAmount(
746       uint256 _amount,
747       uint256 _liquidityFeeNum,
748       uint256 _liquidityFeeDen
749:    ) private pure returns (uint256) {

/// @audit Missing: '@param _message'
910     /**
911      * @notice Parses the message and process the transfer
912      * @dev Will mint the tokens if the token originates on a remote origin
913      * @return The message amount
914      * @return The message token
915      * @return The message transfer id
916      */
917     function _reconcileProcessMessage(bytes memory _message)
918       internal
919       returns (
920         uint256,
921         address,
922:        bytes32

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

File: contracts/contracts/core/connext/facets/AssetFacet.sol

/// @audit Missing: '@param _stableSwapPool'
121     /**
122      * @notice Used to add supported assets. This is an admin only function
123      * @dev When whitelisting the canonical asset, all representational assets would be
124      * whitelisted as well. In the event you have a different adopted asset (i.e. PoS USDC
125      * on polygon), you should *not* whitelist the adopted asset. The stable swap pool
126      * address used should allow you to swap between the local <> adopted asset
127      * @param _canonical - The canonical asset to add by id and domain. All representations
128      * will be whitelisted as well
129      * @param _adoptedAssetId - The used asset id for this domain (i.e. PoS USDC for
130      * polygon)
131      */
132     function setupAsset(
133       ConnextMessage.TokenId calldata _canonical,
134       address _adoptedAssetId,
135       address _stableSwapPool
136:    ) external onlyOwner {

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

File: contracts/contracts/core/connext/facets/PortalFacet.sol

/// @audit Missing: '@param _transferId'
71      /**
72       * @notice Used by routers to perform a manual repayment to Aave Portals to cover any outstanding debt
73       * @dev The router must be approved for portal and with enough liquidity, and must be the caller of this
74       * function
75       * @param _local The local asset (what router stores liquidity in)
76       * @param _backingAmount The principle to be paid (in adopted asset)
77       * @param _feeAmount The fee to be paid (in adopted asset)
78       * @param _maxIn The max value of the local asset to swap for the _backingAmount of adopted asset
79       */
80      function repayAavePortal(
81        address _local,
82        uint256 _backingAmount,
83        uint256 _feeAmount,
84        uint256 _maxIn,
85:       bytes32 _transferId

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

File: contracts/contracts/core/connext/facets/RoutersFacet.sol

/// @audit Missing: '@return'
191      * @param _router The relevant router address
192      */
193:    function getRouterApproval(address _router) public view returns (bool) {

/// @audit Missing: '@return'
200      * @param _router The relevant router address
201      */
202:    function getRouterRecipient(address _router) public view returns (address) {

/// @audit Missing: '@return'
210      * @param _router The relevant router address
211      */
212:    function getRouterOwner(address _router) public view returns (address) {

/// @audit Missing: '@return'
220      * @param _router The relevant router address
221      */
222:    function getProposedRouterOwner(address _router) public view returns (address) {

/// @audit Missing: '@return'
229      * @param _router The relevant router address
230      */
231:    function getProposedRouterOwnerTimestamp(address _router) public view returns (uint256) {

/// @audit Missing: '@return'
245      * @param _router The relevant router address
246      */
247:    function getRouterApprovalForPortal(address _router) public view returns (bool) {

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

File: contracts/contracts/core/connext/facets/BaseConnextFacet.sol

/// @audit Missing: '@return'
112      * @param _router The address of the potential remote xApp Router
113      */
114:    function _isRemoteRouter(uint32 _domain, bytes32 _router) internal view returns (bool) {

/// @audit Missing: '@param _potentialReplica'
136     /**
137      * @notice Determine whether _potentialReplcia is an enrolled Replica from the xAppConnectionManager
138      * @return True if _potentialReplica is an enrolled Replica
139      */
140:    function _isReplica(address _potentialReplica) internal view returns (bool) {

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

File: contracts/contracts/core/connext/helpers/Executor.sol

/// @audit Missing: '@return'
111      * @param _args ExecutorArgs to function.
112      */
113:    function execute(ExecutorArgs memory _args) external payable override onlyConnext returns (bool, bytes memory) {

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

File: contracts/contracts/core/connext/helpers/LPToken.sol

/// @audit Missing: '@return'
19       * @param symbol symbol of this token
20       */
21:     function initialize(string memory name, string memory symbol) external initializer returns (bool) {

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

File: contracts/contracts/core/connext/helpers/StableSwap.sol

/// @audit Missing: '@return'
317      * @param deadline latest timestamp to accept this transaction
318      */
319     function swap(
320       uint8 tokenIndexFrom,
321       uint8 tokenIndexTo,
322       uint256 dx,
323       uint256 minDy,
324       uint256 deadline
325:    ) external override nonReentrant whenNotPaused deadlineCheck(deadline) returns (uint256) {

/// @audit Missing: '@return'
334      * @param minAmountOut the min amount of tokens the user wants to swap to
335      */
336     function swapExact(
337       uint256 amountIn,
338       address assetIn,
339       address assetOut,
340       uint256 minAmountOut
341:    ) external payable override nonReentrant whenNotPaused returns (uint256) {

/// @audit Missing: '@return'
352      * @param maxAmountIn the max amount of tokens the user wants to swap from
353      */
354     function swapExactOut(
355       uint256 amountOut,
356       address assetIn,
357       address assetOut,
358       uint256 maxAmountIn
359:    ) external payable override nonReentrant whenNotPaused returns (uint256) {

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

File: contracts/contracts/core/connext/libraries/AssetLogic.sol

/// @audit Missing: '@return'
31       * @param canonicalId the canonical token id
32       */
33:     function stableSwapPoolExist(bytes32 canonicalId) internal view returns (bool) {

/// @audit Missing: '@param _maxIn'
218     /**
219      * @notice Swaps a local nomad asset for the adopted asset using the stored stable swap
220      * @dev Will not swap if the asset passed in is the adopted asset
221      * @param _asset - The address of the local asset to swap into the adopted asset
222      * @param _amount - The exact amount to receive out of the swap
223      * @return The amount of local asset put into  swap
224      * @return The address of asset received post-swap
225      */
226     function swapFromLocalAssetIfNeededForExactOut(
227       address _asset,
228       uint256 _amount,
229       uint256 _maxIn
230     )
231       internal
232       returns (
233         bool,
234         uint256,
235:        address

/// @audit Missing: '@param _slippageTol'
252     /**
253      * @notice Swaps assetIn t assetOut using the stored stable swap or internal swap pool
254      * @dev Will not swap if the asset passed in is the adopted asset
255      * @param _canonicalId - The canonical token id
256      * @param _assetIn - The address of the from asset
257      * @param _assetOut - The address of the to asset
258      * @param _amount - The amount of the local asset to swap
259      * @return The amount of assetOut
260      * @return The address of assetOut
261      */
262     function _swapAsset(
263       bytes32 _canonicalId,
264       address _assetIn,
265       address _assetOut,
266       uint256 _amount,
267       uint256 _slippageTol
268:    ) internal returns (uint256, address) {

/// @audit Missing: '@param _maxIn'
294     /**
295      * @notice Swaps assetIn t assetOut using the stored stable swap or internal swap pool
296      * @dev Will not swap if the asset passed in is the adopted asset
297      * @param _canonicalId - The canonical token id
298      * @param _assetIn - The address of the from asset
299      * @param _assetOut - The address of the to asset
300      * @param _amountOut - The amount of the _assetOut to swap
301      * @return The amount of assetIn
302      * @return The address of assetOut
303      */
304     function _swapAssetOut(
305       bytes32 _canonicalId,
306       address _assetIn,
307       address _assetOut,
308       uint256 _amountOut,
309       uint256 _maxIn
310     )
311       internal
312       returns (
313         bool,
314         uint256,
315:        address

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/AssetLogic.sol#L31-L33

File: contracts/contracts/core/connext/libraries/SwapUtils.sol

/// @audit Missing: '@param totalSupply'
166     /**
167      * @notice Calculate the dy of withdrawing in one token
168      * @param self Swap struct to read from
169      * @param tokenIndex which token will be withdrawn
170      * @param tokenAmount the amount to withdraw in the pools precision
171      * @return the d and the new y after withdrawing one token
172      */
173     function calculateWithdrawOneTokenDY(
174       Swap storage self,
175       uint8 tokenIndex,
176       uint256 tokenAmount,
177       uint256 totalSupply
178     )
179       internal
180       view
181       returns (
182         uint256,
183         uint256,
184:        uint256

/// @audit Missing: '@param balances'
470     /**
471      * @notice Internally calculates a swap between two tokens.
472      *
473      * @dev The caller is expected to transfer the actual amounts (dx and dy)
474      * using the token contracts.
475      *
476      * @param self Swap struct to read from
477      * @param tokenIndexFrom the token to sell
478      * @param tokenIndexTo the token to buy
479      * @param dx the number of tokens to sell. If the token charges a fee on transfers,
480      * use the amount that gets transferred after the fee.
481      * @return dy the number of tokens the user will get
482      * @return dyFee the associated fee
483      */
484     function _calculateSwap(
485       Swap storage self,
486       uint8 tokenIndexFrom,
487       uint8 tokenIndexTo,
488       uint256 dx,
489       uint256[] memory balances
490:    ) internal view returns (uint256 dy, uint256 dyFee) {

/// @audit Missing: '@param balances'
501     /**
502      * @notice Internally calculates a swap between two tokens.
503      *
504      * @dev The caller is expected to transfer the actual amounts (dx and dy)
505      * using the token contracts.
506      *
507      * @param self Swap struct to read from
508      * @param tokenIndexFrom the token to sell
509      * @param tokenIndexTo the token to buy
510      * @param dy the number of tokens to buy. If the token charges a fee on transfers,
511      * use the amount that gets transferred after the fee.
512      * @return dx the number of tokens the user have to deposit
513      * @return dxFee the associated fee
514      */
515     function _calculateSwapInv(
516       Swap storage self,
517       uint8 tokenIndexFrom,
518       uint8 tokenIndexTo,
519       uint256 dy,
520       uint256[] memory balances
521:    ) internal view returns (uint256 dx, uint256 dxFee) {

/// @audit Missing: '@param self'
536     /**
537      * @notice A simple method to calculate amount of each underlying
538      * tokens that is returned upon burning given amount of
539      * LP tokens
540      *
541      * @param amount the amount of LP tokens that would to be burned on
542      * withdrawal
543      * @return array of amounts of tokens user will receive
544      */
545:    function calculateRemoveLiquidity(Swap storage self, uint256 amount) internal view returns (uint256[] memory) {

/// @audit Missing: '@return'
623      * @param numTokens number of tokens pooled
624      */
625:    function _feePerToken(uint256 swapFee, uint256 numTokens) internal pure returns (uint256) {

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

File: contracts/contracts/core/connext/libraries/LibCrossDomainProperty.sol

/// @audit Missing: '@return'
155      * @param _property The bytes representation of the property
156      */
157:    function parseDomainAndSenderBytes(bytes memory _property) internal pure returns (bytes29) {

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibCrossDomainProperty.sol#L155-L157

File: contracts/contracts/core/promise/PromiseRouter.sol

/// @audit Missing: '@param _message'
226     /**
227      * @notice Process stored callback function
228      * @param transferId The transferId to process
229      */
230:    function process(bytes32 transferId, bytes calldata _message) public nonReentrant {

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L226-L230

14. Event is missing indexed fields

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

There are 69 instances of this issue:

File: contracts/contracts/core/connext/facets/BridgeFacet.sol

75      event XCalled(
76        bytes32 indexed transferId,
77        XCallArgs xcallArgs,
78        XCalledEventArgs args,
79        uint256 nonce,
80        bytes message,
81        address caller
82:     );

93      event Reconciled(
94        bytes32 indexed transferId,
95        uint32 indexed origin,
96        address[] routers,
97        address asset,
98        uint256 amount,
99        address caller
100:    );

114     event Executed(
115       bytes32 indexed transferId,
116       address indexed to,
117       ExecuteArgs args,
118       address transactingAsset,
119       uint256 transactingAmount,
120       address caller
121:    );

129:    event TransferRelayerFeesUpdated(bytes32 indexed transferId, uint256 relayerFee, address caller);

139     event ForcedReceiveLocal(
140       bytes32 indexed transferId,
141       bytes32 indexed canonicalId,
142       uint32 canonicalDomain,
143       uint256 amount
144:    );

153:    event AavePortalMintUnbacked(bytes32 indexed transferId, address indexed router, address asset, uint256 amount);

162:    event AavePortalRepayment(bytes32 indexed transferId, address asset, uint256 amount, uint256 fee);

171:    event AavePortalRepaymentDebt(bytes32 indexed transferId, address asset, uint256 amount, uint256 fee);

179:    event SponsorVaultUpdated(address oldSponsorVault, address newSponsorVault, address caller);

187:    event PromiseRouterUpdated(address oldRouter, address newRouter, address caller);

195:    event ExecutorUpdated(address oldExecutor, address newExecutor, address caller);

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

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

62:     event AssetRemoved(bytes32 canonicalId, address caller);

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

File: contracts/contracts/core/connext/facets/PortalFacet.sol

30:     event AavePortalRouterRepayment(address indexed router, address asset, uint256 amount, uint256 fee);

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

File: contracts/contracts/core/connext/facets/RelayerFacet.sol

25:     event RelayerFeeRouterUpdated(address oldRouter, address newRouter, address caller);

32:     event RelayerAdded(address relayer, address caller);

39:     event RelayerRemoved(address relayer, address caller);

48:     event InitiatedClaim(uint32 indexed domain, address indexed recipient, address caller, bytes32[] transferIds);

56:     event Claimed(address indexed recipient, uint256 total, bytes32[] transferIds);

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

File: contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol

52:     event RouterOwnershipRenunciationProposed(uint256 timestamp);

54:     event RouterOwnershipRenounced(bool renounced);

56:     event AssetOwnershipRenunciationProposed(uint256 timestamp);

58:     event AssetOwnershipRenounced(bool renounced);

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

File: contracts/contracts/core/connext/facets/RoutersFacet.sol

65:     event RouterAdded(address indexed router, address caller);

72:     event RouterRemoved(address indexed router, address caller);

103:    event MaxRoutersPerTransferUpdated(uint256 maxRoutersPerTransfer, address caller);

110:    event LiquidityFeeNumeratorUpdated(uint256 liquidityFeeNumerator, address caller);

117:    event RouterApprovedForPortal(address router, address caller);

124:    event RouterUnapprovedForPortal(address router, address caller);

133     event RouterLiquidityAdded(
134       address indexed router,
135       address local,
136       bytes32 canonicalId,
137       uint256 amount,
138       address caller
139:    );

149:    event RouterLiquidityRemoved(address indexed router, address to, address local, uint256 amount, address caller);

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

File: contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol

65:     event NewAdmin(address oldAdmin, address newAdmin);

66:     event PriceRecordUpdated(address token, address baseToken, address lpToken, bool _active);

67:     event DirectPriceUpdated(address token, uint256 oldPrice, uint256 newPrice);

68:     event AggregatorUpdated(address tokenAddress, address source);

69:     event V1PriceOracleUpdated(address oldAddress, address newAddress);

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

File: contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol

62:     event RouterOwnershipRenunciationProposed(uint256 timestamp);

64:     event RouterOwnershipRenounced(bool renounced);

66:     event AssetOwnershipRenunciationProposed(uint256 timestamp);

68:     event AssetOwnershipRenounced(bool renounced);

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

File: contracts/contracts/core/connext/helpers/SponsorVault.sol

73:     event ConnextUpdated(address oldConnext, address newConnext, address caller);

78:     event RateUpdated(uint32 originDomain, Rate oldRate, Rate newRate, address caller);

83:     event RelayerFeeCapUpdated(uint256 oldRelayerFeeCap, uint256 newRelayerFeeCap, address caller);

88:     event GasTokenOracleUpdated(address oldOracle, address newOracle, address caller);

93:     event TokenExchangeUpdated(address token, address oldTokenExchange, address newTokenExchange, address caller);

98:     event ReimburseLiquidityFees(address token, uint256 amount, address receiver);

103:    event ReimburseRelayerFees(uint256 amount, address receiver);

108:    event Deposit(address token, uint256 amount, address caller);

113:    event Withdraw(address token, address receiver, uint256 amount, address caller);

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

File: contracts/contracts/core/connext/libraries/AmplificationUtils.sol

17:     event RampA(uint256 oldA, uint256 newA, uint256 initialTime, uint256 futureTime);

18:     event StopRampA(uint256 currentA, uint256 time);

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

File: contracts/contracts/core/connext/libraries/SwapUtils.sol

25:     event TokenSwap(address indexed buyer, uint256 tokensSold, uint256 tokensBought, uint128 soldId, uint128 boughtId);

26      event AddLiquidity(
27        address indexed provider,
28        uint256[] tokenAmounts,
29        uint256[] fees,
30        uint256 invariant,
31        uint256 lpTokenSupply
32:     );

33:     event RemoveLiquidity(address indexed provider, uint256[] tokenAmounts, uint256 lpTokenSupply);

34      event RemoveLiquidityOne(
35        address indexed provider,
36        uint256 lpTokenAmount,
37        uint256 lpTokenSupply,
38        uint256 boughtId,
39        uint256 tokensBought
40:     );

41      event RemoveLiquidityImbalance(
42        address indexed provider,
43        uint256[] tokenAmounts,
44        uint256[] fees,
45        uint256 invariant,
46        uint256 lpTokenSupply
47:     );

48:     event NewAdminFee(uint256 newAdminFee);

49:     event NewSwapFee(uint256 newSwapFee);

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

File: contracts/contracts/core/connext/libraries/LibDiamond.sol

69:     event DiamondCutProposed(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata, uint256 deadline);

81:     event DiamondCutRescinded(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata);

92:     event DiamondCut(IDiamondCut.FacetCut[] _diamondCut, address _init, bytes _calldata);

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

File: contracts/contracts/core/promise/PromiseRouter.sol

78      event Send(
79        uint32 domain,
80        bytes32 remote,
81        bytes32 transferId,
82        address callbackAddress,
83        bool success,
84        bytes data,
85        bytes message
86:     );

99      event Receive(
100       uint64 indexed originAndNonce,
101       uint32 indexed origin,
102       bytes32 transferId,
103       address callbackAddress,
104       bool success,
105       bytes data,
106       bytes message
107:    );

116:    event CallbackFeeAdded(bytes32 indexed transferId, uint256 addedFee, uint256 totalFee, address caller);

123:    event CallbackExecuted(bytes32 indexed transferId, address relayer);

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L78-L86

File: contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol

50:     event Send(uint32 domain, address recipient, bytes32[] transferIds, bytes32 remote, bytes message);

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol#L50

15. Not using the named return variables anywhere in the function is confusing

Consider changing the variable to be an unnamed one

There are 2 instances of this issue:

File: contracts/contracts/core/connext/facets/StableSwapFacet.sol   #1

/// @audit availableTokenAmount
208     function calculateRemoveSwapLiquidityOneToken(
209       bytes32 canonicalId,
210       uint256 tokenAmount,
211       uint8 tokenIndex
212:    ) external view returns (uint256 availableTokenAmount) {

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

File: contracts/contracts/core/connext/helpers/StableSwap.sol   #2

/// @audit availableTokenAmount
291     function calculateRemoveLiquidityOneToken(uint256 tokenAmount, uint8 tokenIndex)
292       external
293       view
294       override
295:      returns (uint256 availableTokenAmount)

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L291-L295
@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 19, 2022
code423n4 added a commit that referenced this issue Jun 19, 2022
@code423n4
IllIllI issue #186
7826c65
@jakekidd
Copy link
Collaborator

jakekidd commented Jul 2, 2022

really appreciate the formatting here!! props to this auditor

@jakekidd
Copy link
Collaborator

jakekidd commented Jul 2, 2022

Low risk:
1 is invalid - common practice here
2+4 are iffy
5 is not low risk, no errors are specified, but it is QA

QA:
1,2,4 are invalid

@0xleastwood 0xleastwood mentioned this issue Aug 12, 2022
Closed
@IllIllI000
Copy link

@0xleastwood can you compare and contrast this report with this other one? #236 It's not clear why the other one is ranked higher even though there are more issues here

@IllIllI000
Copy link

IllIllI000 commented Aug 20, 2022 •
edited
Loading

Low risk: 1 is invalid - common practice here 2+4 are iffy 5 is not low risk, no errors are specified, but it is QA

QA: 1,2,4 are invalid

L-1: common practice does not mean safe. At one point use of payable.transfer was common, and is now considered unsafe. Having an open payable receive() means uses may send funds which are then inaccessible. Loss of funds is not invalid
L-2: It's deprecated because of the risks involved, as is seen by the Medium bugs identified in this contest related to such calls
L-4: There is no compelling reason, and the documentation says there should be one
L-5: If these comments had been addressed, it's possible some of the other issues wardens filed would not have been flagged: Should we call approve(0) and approve(totalRepayAmount) instead? or with a try catch
N-1: Not invalid - see this report code-423n4/2022-06-badger-findings#118 (comment)
N-2: Is the file used somewhere that I missed? If not, I don't see how it's invalid
N-4: Can you elaborate on why you believe these to be invalid? The right tool should be used for the job, not one that happens to work

@0xleastwood
Copy link
Collaborator

0xleastwood commented Aug 24, 2022 •
edited
Loading

@0xleastwood can you compare and contrast this report with this other one? #236 It's not clear why the other one is ranked higher even though there are more issues here

While your report may have more findings, most of them are non-critical which are technically not rewarded but I always like to take them into consideration because the majority of them are helpful. #236 contained a severe issue related to the unsafe use of safeApprove which did not satisfy the medium severity level because the impact was not clearly outlined. But nonetheless, the impact is severe and I weighed that report heavily in favour of that single issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jakekidd @0xleastwood @code423n4 @IllIllI000