Connext Amarok contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

In StableSwapFacet, users can swap tokens in case of emergencies and this could lead to unexpected results.

Proof of Concept

According to L21-L23, swap must be paused in case of emergencies. But StableSwapFacet.swapExactOut() can work as it doesn't have a "whenNotPaused" modifier and it could lead to unexpected results.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

"whenNotPaused" must be added to StableSwapFacet.swapExactOut().

Summary

I've found 7 low-risk issues and some non-critical ones.

Low-Risk Issues

1. Wrong comments 2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L223 "@notice Calculate the amount of tokens the user must transfer"

2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L227 "@return amount of tokens the user must transfer"

2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L240 "@notice Calculate the amount of tokens the user must transfer"

2022-06-connext\contracts\contracts\core\connext\libraries\LibConnextStorage.sol#L299 Currently, it's the same as "portalDebt" and it must be changed for "portalFeeDebt".

2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L209 "// xpReduced[i] = xp[i] - dxExpected * fee / FEE_DENOMINATOR"

2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L790 "Swap needs more than max tokens"

2. safeApprove() is deprecated, use safeIncreaseAllowance() and safeDecreaseAllowance() instead 2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L347

3. Missing events for onlyAdmin functions that change important parameters 2022-06-connext\contracts\contracts\core\connext\facets\PortalFacet.sol#L68

4. Check address(0) before transfer funds even if they are onlyAdmin functions. 2022-06-connext\contracts\contracts\core\connext\facets\RelayerFacet.sol#L172 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L260 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L293 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L296 2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L142 2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L145 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1059

5. PortalFacet.getAavePortalFeeDebt() returns wrong results. 2022-06-connext\contracts\contracts\core\connext\facets\PortalFacet.sol#L39 It must return "s.portalFeeDebt[_transferId]".

6. Check address(0) when change admin role. 2022-06-connext\contracts\contracts\core\connext\helpers\ConnextPriceOracle.sol#L168

7. Inconsistent implementation of "adminFee" and "swapFee". 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1071 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1084 2022-06-connext\contracts\contracts\core\connext\facets\StableSwapFacet.sol#L432-L433 2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L97-98

It checks adminFee < MAX_ADMIN_FEE, swapFee < MAX_SWAP_FEE in 2 places but checks adminFee <= MAX_ADMIN_FEE, swapFee <= MAX_SWAP_FEE in other place.

Non-critical Issues

1. Natspec incomplete 2022-06-connext\contracts\contracts\core\connext\facets\AssetFacet.sol#L132 @param _stableSwapPool

2022-06-connext\contracts\contracts\core\connext\facets\PortalFacet.sol#L80 @param _transferId

2022-06-connext\contracts\contracts\core\connext\facets\StableSwapFacet.sol#L255 @param minAmountOut @param deadline @return

2022-06-connext\contracts\contracts\core\connext\facets\StableSwapFacet.sol#L279 @param maxAmountIn @param deadline @return

2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L319 @return

2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L336 @return

2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L354 @return

2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L226 @param _maxIn @return

2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L262 @param _slippageTol @return

2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L304 @param _maxIn @return

2022-06-connext\contracts\contracts\core\connext\libraries\LibConnextStorage.sol#L77 @param routerSignatures

2022-06-connext\contracts\contracts\core\connext\libraries\LibConnextStorage.sol#L100 @param approvedForPortalRouters

2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L173 @param totalSupply

2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L484 @param balances

2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L515 @param balances

2. Event should use indexed fields if there are three or more fields 2022-06-connext\contracts\contracts\core\connext\facets\AssetFacet.sol#L27 2022-06-connext\contracts\contracts\core\connext\facets\AssetFacet.sol#L35 2022-06-connext\contracts\contracts\core\connext\facets\AssetFacet.sol#L44 2022-06-connext\contracts\contracts\core\connext\facets\AssetFacet.sol#L55 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L179 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L187 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L195 2022-06-connext\contracts\contracts\core\connext\facets\RelayerFacet.sol#L25 2022-06-connext\contracts\contracts\core\connext\helpers\ConnextPriceOracle.sol#L66-L67 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L73 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L78 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L83 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L88 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L93 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L98 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L108 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L113 2022-06-connext\contracts\contracts\core\connext\libraries\AmplificationUtils.sol#L17 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L69 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L81 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L92 2022-06-connext\contracts\contracts\core\promise\PromiseRouter.sol#L78 2022-06-connext\contracts\contracts\core\relayer-fee\RelayerFeeRouter.sol#L50

3. Typo 2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L244 @return amount of tokens the user "must(has to)" transfer

2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L253 @notice Swaps assetIn "t" assetOut using the stored stable swap or internal swap pool

2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L295 @notice Swaps assetIn "t" assetOut using the stored stable swap or internal swap pool

2022-06-connext\contracts\contracts\core\connext\libraries\AssetLogic.sol#L331 NOTE: this is less efficient "then" relying on the swapInternalOut revert, but makes it easier

2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L917 minAmounts must match "poolTokens" => "pooledTokens"?

4. Needless import 2022-06-connext\contracts\contracts\core\connext\helpers\ConnextPriceOracle.sol#L4 2022-06-connext\contracts\contracts\core\connext\libraries\AmplificationUtils.sol#L5 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L4

5. Immutable addresses should be 0-checked 2022-06-connext\contracts\contracts\core\connext\helpers\Executor.sol#L48

6. Open TODOs 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L492 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L579 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L1027 2022-06-connext\contracts\contracts\core\connext\helpers\Executor.sol#L7 2022-06-connext\contracts\contracts\core\connext\libraries\LibConnextStorage.sol#L303

1. Use != 0 instead of > 0 for uint variables 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L499 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L665 2022-06-connext\contracts\contracts\core\connext\facets\StableSwapFacet.sol#L416 2022-06-connext\contracts\contracts\core\connext\helpers\ConnextPriceOracle.sol#L150 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L217 2022-06-connext\contracts\contracts\core\connext\libraries\AmplificationUtils.sol#L86 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L121 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L139 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L158 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L226 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L232 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L247 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L369 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L670 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L711 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L765 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L799 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L845 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L965 2022-06-connext\contracts\contracts\core\promise\PromiseRouter.sol#L259

2. Use ++i instead of i++, i+=1, also unchecked increments in for-loops will save gas cost 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L613 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L684 2022-06-connext\contracts\contracts\core\connext\facets\BridgeFacet.sol#L799 2022-06-connext\contracts\contracts\core\connext\facets\DiamondLoupeFacet.sol#L31 2022-06-connext\contracts\contracts\core\connext\facets\RelayerFacet.sol#L144 2022-06-connext\contracts\contracts\core\connext\facets\RelayerFacet.sol#L168 2022-06-connext\contracts\contracts\core\connext\facets\StableSwapFacet.sol#L415 2022-06-connext\contracts\contracts\core\connext\helpers\ConnextPriceOracle.sol#L176 2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L81 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L104 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L129 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L134 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L147 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L153 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L162 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L205 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L254 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L268 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L289 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L300 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L302 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L344 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L405 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L425 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L558 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L591 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L844 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L869 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L924 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1014 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1019 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1039 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1055 2022-06-connext\contracts\contracts\core\relayer-fee\libraries\RelayerFeeMessage.sol#L85

3. Non-strict inequalities are cheaper than strict ones 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L214 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L256 2022-06-connext\contracts\contracts\core\connext\helpers\SponsorVault.sol#L258 2022-06-connext\contracts\contracts\core\connext\libraries\AmplificationUtils.sol#L59 2022-06-connext\contracts\contracts\core\connext\libraries\MathUtils.sol#L29

4. Check require(), assert() at the beginning of function 2022-06-connext\contracts\contracts\core\connext\helpers\LPToken.sol#L50 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L123 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L141 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L161

5. No need to initialize variables with default values 2022-06-connext\contracts\contracts\core\relayer-fee\libraries\RelayerFeeMessage.sol#L81 2022-06-connext\contracts\contracts\core\connext\facets\StableSwapFacet.sol#L415 2022-06-connext\contracts\contracts\core\connext\helpers\ConnextPriceOracle.sol#L176 2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L81 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L205 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L254 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L268 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L289 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L300 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L302 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L344 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L405 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L425 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L558 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L591 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L844 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L869 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L924 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1014 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1019 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1039 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1055

6. Don't use storage value if possible 2022-06-connext\contracts\contracts\core\connext\facets\PortalFacet.sol#L108

s.routerBalances[msg.sender][_local] = routerBalance - amountIn;

7. An array’s length should be cached to save gas in for-loops 2022-06-connext\contracts\contracts\core\connext\facets\RelayerFacet.sol#L140 2022-06-connext\contracts\contracts\core\connext\facets\RelayerFacet.sol#L164 2022-06-connext\contracts\contracts\core\connext\facets\StableSwapFacet.sol#L415 2022-06-connext\contracts\contracts\core\connext\helpers\ConnextPriceOracle.sol#L176 2022-06-connext\contracts\contracts\core\connext\helpers\StableSwap.sol#L81 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L104 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L129 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L147 2022-06-connext\contracts\contracts\core\connext\libraries\LibDiamond.sol#L162 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L205 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L558 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L591 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L844 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L869 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L924 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1014 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1019 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1039 2022-06-connext\contracts\contracts\core\connext\libraries\SwapUtils.sol#L1055