Platform: Code4rena
Start Date: 08/06/2022
Pot Size: $115,000 USDC
Total HM: 26
Participants: 72
Period: 11 days
Judge: leastwood
Total Solo HM: 14
Id: 132
League: ETH
Rank: 36/72
Findings: 2
Award: $241.00
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: BowTiedWardens
Also found by: 0x1f8b, 0x29A, 0x52, 0xNazgul, 0xNineDec, 0xf15ers, 0xkatana, 0xmint, Chom, ElKu, Funen, IllIllI, JMukesh, Jujic, Kaiziron, Lambda, MiloTruck, Ruhum, SmartSek, SooYa, TerrierLover, TomJ, WatchPug, Waze, _Adam, asutorufos, auditor0517, bardamu, c3phas, catchup, cccz, ch13fd357r0y3r, cloudjunky, cmichel, cryptphi, csanuragjain, defsec, fatherOfBlocks, hansfriese, hyh, jayjonah8, joestakey, k, kenta, obtarian, oyc_109, robee, sach1r0, shenwilly, simon135, slywaters, sorrynotsorry, tintin, unforgiven, xiaoming90, zzzitron
141.8225 USDC - $141.82
#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.
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
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.
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
Tool Manual review
Recommendation Mitigation Steps Add indexed at transferId.
#0 - jakekidd
2022-07-02T00:36:16Z
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
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, 0xmint, BowTiedWardens, ElKu, Fitraldys, Funen, Kaiziron, Lambda, Metatron, MiloTruck, Randyyy, Ruhum, SmartSek, TomJ, Tomio, UnusualTurtle, Waze, _Adam, apostle0x01, asutorufos, c3phas, catchup, csanuragjain, defsec, fatherOfBlocks, hansfriese, hyh, ignacio, joestakey, k, kaden, nahnah, oyc_109, rfa, robee, sach1r0, simon135, slywaters
99.18 USDC - $99.18
#1 Use storage instead memory
Use storage instead of memory to reduce the gas fee. I suggest change
ConnextMessage.TokenId memory canonical = ConnextMessage.TokenId(
to
ConnextMessage.TokenId storage canonical = ConnextMessage.TokenId(
apply to others.
#2 Caching s.adoptedToCanonical
cache the s.adoptedToCanonical to the memory for reduce the gas fee because it use multiple times.
s.adoptedToCanonical[supported].domain = _canonical.domain; s.adoptedToCanonical[supported].id = _canonical.id;
#3 Sort the struct XCalledEventArgs
sort the struct can reduce the gas fee, i suggest to change from
struct XCalledEventArgs { address transactingAssetId; uint256 amount; uint256 bridgedAmt; address bridged;
to
struct XCalledEventArgs { uint256 amount; uint256 bridgedAmt; address transactingAssetId; address bridged;
#4 Visibility
change visibility from public to private can save gas. so i recommend to change it.
#5 Short the error message
reduce size of error message to bytes 32 can reduce the gas fee. reduce that if possible.
#6 Pre increment
using pre increment more cheaper gas than post increment. so, i sugget to change from
selectorPosition++;
to
++selectorPosition;
apply it to others.
#7 Default uint is 0
the default value of uint is 0, so remove unnecassary explicit code.
#8 inequality
non strict inequality are cheaper than strict one. i suggest to use >= or <= instead of > and < if possible.
#9 Use calldata
In the external functions where the function argument is read-only, the function() has an inputed parameter that using memory, if this function didnt change the parameter, its cheaper to use calldata then memory. so we suggest to change it.
#10 Cache _transferIds
cache the _targetIds.length can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity and it use twice.
#11 Cache tokenAddresses
caching the tokenAddresses.length can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity and it use twice.
#12 Cache pooled and decimal
caching the pooled.length and decimal.length can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity and it use twice.
#13 Use !=0 instead >=0
for unsigned integer, >0 is less efficient then !=0, so use !=0 instead of >0. apply to others.
#14 cache _pooledTokens
caching _pooledTokens.length to memory because use multiple times can reduce the gas.
#15 Cache _a * AmplificationUtils.A_PRECISION;
swapStorage.initialA = _a * AmplificationUtils.A_PRECISION; swapStorage.futureA = _a * AmplificationUtils.A_PRECISION;
cache the_a * AmplificationUtils.A_PRECISION; to memory because use multiple times can reduce the gas.
#16 Sort callparam struct
#17 Sort executearg struct
#18 Cache calldata.length
caching _calldata.length to memory because use multiple times can reduce the gas.
#19 Caching _facetAddress
caching _facetAddress to memory can reduce the gas. Because use multiple times.
#20 Caching xp.length
caching xp.length to memory because use multiple times can reduce the gas.