Connext Amarok contest - Funen's results

1. Title : Innacurate comment with actual code

return was used as comment but in actual code no return was used for _remote The address of the remote xApp Router on _domain. It can be added or it can be deleted since it was there.

Tool Used

Manual Review

Recommended Mitigation

Add return inside function

2. Title : Missmatch comment with actual code

In this line was used to be :

   * @return Returns (`_origin` << 32) & `_nonce`

but in actual code was using and OR :

       return (uint64(_origin) << 32) | _nonce;

So it can be changed as it should be

POC

https://www.geeksforgeeks.org/solidity-operators/

Another Occurance

RelayerFeeRouter.sol Line.166

3. Title : Update your code since solidity ver 0.8.0

Optional: If you use SafeMath or a similar library, change x.add(y) to x + y, x.mul(y) to x * y etc.

POC

https://docs.soliditylang.org/en/v0.8.15/080-breaking-changes.html

1.) File : AmplificationUtils.sol Line.61

2.) File : AmplificationUtils.sol Line.61

3.) File : AmplificationUtils.sol Line.64

4.) File : AmplificationUtils.sol Line.89

5.) File : AmplificationUtils.sol Line.92

6.) File : AmplificationUtils.sol Line.92

4. Title : Avoid floating pragmas: the version should be locked

The pragma declared at DiamondInit.sol was used ^0.8.0. As the compiler can be use as 0.8.14 and consider locking at this version the same as another. It can be consider using locking the pragma version whenever possible and avoid using a floating pragma in the final deployment. Since it can be problematic, if there are publicly disclosed bugs and issues that affect the current compiler version used.

Tool Used

Manual Review

5. Change var name code for good readibility

In Executor.sol, since amnt for this was used for amount, it better to keep as it should be for good readibility and code instead. No exploit was happen but to keep it good as it should be. Since another amount was not be abbreviated. so it should be better that to keep it amount and not amnt.

Tool used

Manual Review

Occurances

1)Executor.sol #L31 2)Executor.sol #L102 3)Executor.sol #L153 4)Executor.sol #L172

6. Title : NatSpec is incomplete

1.) File : BaseConnextFacet.sol Lines 109-116

  /**
   * @notice Return true if the given domain / router is the address of a remote xApp Router
   * @param _domain The domain of the potential remote xApp Router
   * @param _router The address of the potential remote xApp Router
   */
  function _isRemoteRouter(uint32 _domain, bytes32 _router) internal view returns (bool) {
    return s.remotes[_domain] == _router;
  }

adding @return

2.) File : BridgeFacet.sol Lines.477-524

adding @params & @return

3.) File : BridgeFacet.sol Lines.622-630

adding @return

4.) File : BridgeFacet.sol Lines.632-700

adding @return

5.) File : BridgeFacet.sol Lines.702-713

moving // return into @return

6.) File : BridgeFacet.sol Lines.715-722

adding @return

7.) File : BridgeFacet.sol Lines.724-737

adding @return

8.) File : BridgeFacet.sol Lines.739-751

adding @return

9.) File : BridgeFacet.sol Lines.739-751

adding @return

10. File : BridgeFacet.sol Lines.815-880

adding @return

11. File : BridgeFacet.sol Lines.739-751

adding @return

12.) File : LPToken.sol Lines.14-26

adding @return

7. Title : Unnecessary Declared Comment

1.) File : contracts/core/connext/facets/StableSwapFacet.sol Line. 49

  // ============ Properties ============

This comment was unnecessary so it can be deleted instead since this was unused and pass it into modifier directly.

2.) File : contracts/core/connext/helpers/SponsorVault.sol Line.27

// ============ Private storage ============

This comment was unnecessary so it can be deleted instead since this was unused and pass it into public storage directly.

3.) Since this comment was used to be an dev commented for the code but it can be deleted instead for better code readibility since it was unnecesary.

4.) This a la from function originsender() was unused and misslead so it can be changed or it can be deleted instead

another occurances : Executor.sol Line 85 & Line 98

Tool Used

Manual Review

8. Title : Typo Comment

1.) File : BaseConnextFacet.sol Line 137

_potentialReplcia into _potentialReplica

2.) File : BaseConnextFacet.sol Line 764

routers' into routers

3.) File : BaseConnextFacet.sol Line 982

amongst into amongs

4.) File : PortalFacet.sol Line 111

back loan into backloan

5.) File : PortalFacet.sol Line 188

back loan into backloan

6.) File : PortalFacet.sol Line.163

sournce into source

7.) File : StableSwapFacet.sol Line 377

users' into users

1. Title : Shorter code can be save more gas

1.) File : contracts/core/connext/facets/BridgeFacet.sol Line. 843

        _amount = _amount + sponsored;

change to :

_amount += sponsored:

2. Title : Saving gas by removing = 0

This implementation code can be saving more gas by removing = 0, it because If a variable was not set/initialized, it is assumed to have default value to 0

Tool Used

Manual Review

Mitigation Step

Remove = 0

Occurances

core/connext/facets/BridgeFacet.sol#L1103        backUnbackedDebt = 0;
core/connext/facets/BridgeFacet.sol#L1108        portalFee = 0;
contracts/core/connext/facets/BridgeFacet.sol#L1046      amountIn = 0;
contracts/core/connext/facets/PortalFacet.sol#L154        _feeAmount = 0;
contracts/core/connext/facets/ProposedOwnableFacet.sol#L272    s._routerOwnershipTimestamp = 0;
contracts/core/connext/facets/ProposedOwnableFacet.sol#L283    s._assetOwnershipTimestamp = 0;
contracts/core/connext/facets/ProposedOwnableFacet.sol#L288    s._proposedOwnershipTimestamp = 0;

3. Title : Using short reason string can be used for saving more gas

Every reason string takes at least 32 bytes. Use short reason strings that fits in 32 bytes or it will become more expensive.

Tool Used

Manual Review

Occurances

contracts/core/connext/libraries/SwapUtils.sol#L342 "Balances must match multipliers"
contracts/core/connext/libraries/SwapUtils.sol#L595 "Cannot withdraw more than available"
contracts/core/connext/libraries/SwapUtils.sol#L649 "Cannot swap more than you own"
contracts/core/connext/libraries/SwapUtils.sol#L662 "Swap didn't result in min tokens"
contracts/core/connext/libraries/SwapUtils.sol#L697 "Cannot get more than pool balance"
contracts/core/connext/libraries/SwapUtils.sol#L703 "Swap needs more than max tokens"
contracts/core/connext/libraries/SwapUtils.sol#L716 "Cannot swap more than you own"
contracts/core/connext/libraries/SwapUtils.sol#L750 "Cannot swap more than you own"
contracts/core/connext/libraries/SwapUtils.sol#L756 "Swap didn't result in min tokens"
contracts/core/connext/libraries/SwapUtils.sol#L784 "Cannot get more than pool balance"
contracts/core/connext/libraries/SwapUtils.sol#L790 "Swap didn't result in min tokens"
contracts/core/connext/libraries/SwapUtils.sol#L823 "Amounts must match pooled tokens"
contracts/core/connext/libraries/SwapUtils.sol#L917 "minAmounts must match poolTokens"
contracts/core/connext/libraries/SwapUtils.sol#L1005 "Amounts should match pool tokens"
contracts/core/connext/libraries/SwapUtils.sol#L1015  "Cannot withdraw more than available"
contracts/core/connext/libraries/AmplificationUtils.sol#L86 "futureA_ must be > 0 and < MAX_A"
contracts/core/connext/libraries/AmplificationUtils.sol#L84 "Wait 1 day before starting ramp"