search

code-423n4 / 2022-06-connext-findings

1 stars 0 forks source link

QA Report #167

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1 Unused variable

Impact efficiency code and reduce gas cost

Proof of Concept https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/BridgeFacet.sol#L859

Tool Used Remix

Recommended Mitigation Steps remove unnecessary code. i suggest to change

  (bool success, bytes memory returnData) = s.executor.execute

to

  (bool success, ) = s.executor.execute

2 Missing param define _transferId

impact repayAavePortal function have natspec comment which is missing the newOwner function parameter. Issues with comments are low risk based on Code4rena risk categories.

Proof of Concept https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/PortalFacet.sol#L74-L79

Tool Manual Review

Recommended Mitigation Steps Add natspec comments include transferId parameter in repayAavePortal function.

3 Router must be indexed

Impact event is missing indexed fields.

Proof of Concept https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L117

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

Tool Manual review

Recommendation Mitigation Steps Add indexed at router.

4 canonicalId must be indexed

Impact event is missing indexed fields.

Proof of Concept https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L136

Tool Manual review

Recommendation Mitigation Steps Add indexed at canonicalId.

5 Missing amount check

Impact Being instantiated with wrong configuration the contract will be inoperable. amount can't be 0.

Proof of Concept https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L490

Tool Manual review

Recommendation Mitigation Steps add 

if (_amount == 0) revert()

to the function

6 Typo

Impact Misleading the user

Proof of Concept https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L575

Tool Manual Review

Recommended Mitigation Steps Fix it from specicfied to specified.

7 inclusive condition

Impact this functions will fail when the end-user expects it to pass through

Proof of Concept https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/facets/RoutersFacet.sol#L592

Tool Manual Review

RecommendationMitigation Steps Conditions routerBalance should be inclusive >= or <= :

8 Param wrapped must be immutable

Impact the state wrapped can't be initialize by constructor.

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

Tool Used Manual Review

Recommended Mitigation Steps the state must add immutable because in the constructor parameter mention wrapped to initialize. so i suggest to add immutable on it.

9 transferId must be indexed

Impact event is missing indexed fields.

Proof of Concept https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/promise/PromiseRouter.sol#L102

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

Tool Manual review

Recommendation Mitigation Steps Add indexed at transferId.

jakekidd commented 2 years ago

1 invalid: Unused variable is being used in event emit below? 7 invalid: should NOT be inclusive comparison 8 invalid?: needs more explanation, I think? there may or may not be a wrapper contract on a given chain, and that may change, so it can't be immutable

3+4+9 should be 1 issue