Open code423n4 opened 2 years ago
really appreciate the formatting here!! props to this auditor
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 can you compare and contrast this report with this other one? https://github.com/code-423n4/2022-06-connext-findings/issues/236 It's not clear why the other one is ranked higher even though there are more issues here
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 https://github.com/code-423n4/2022-06-badger-findings/issues/118#issuecomment-1162499207
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 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.
Summary
Low Risk Issues
receive()
/fallback()
functionsafeApprove()
is deprecatedaddress(0x0)
when assigning values toaddress
state variablesabi.encodePacked()
should not be used with dynamic types when passing the result to a hash function such askeccak256()
__gap[50]
storage variable to allow for new storage variables in later versionsTotal: 18 instances over 6 issues
Non-critical Issues
initializer
modifier on constructornonReentrant
modifier
should occur before all other modifierspublic
functions not called by the contract should be declaredexternal
insteadconstant
s should be defined rather than using magic numberskeccak256()
, should useimmutable
rather thanconstant
1e18
) rather than exponentiation (e.g.10**18
)constant
/immutable
variablesindexed
fieldsTotal: 205 instances over 15 issues
Low Risk Issues
1. Unused/empty
receive()
/fallback()
functionIf 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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L132
2.
safeApprove()
is deprecatedDeprecated in favor of
safeIncreaseAllowance()
andsafeDecreaseAllowance()
. If only setting the initial allowance to the value that means infinite,safeIncreaseAllowance()
can be used insteadThere is 1 instance of this issue:
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 toaddress
state variablesThere are 8 instances of this issue:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L77
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L48
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol#L315
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 askeccak256()
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)
, butabi.encode(0x123,0x456)
=>0x0...1230...456
). "Unless there is a compelling reason,abi.encode
should be preferred". If there is only one argument toabi.encodePacked()
it can often be cast tobytes()
orbytes32()
instead.If all arguments are strings and or bytes,
bytes.concat()
should be used insteadThere is 1 instance of this issue:
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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L492
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 versionsSee 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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/LPToken.sol#L13
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 constructorOpenZeppelin recommends that the
initializer
modifier be applied to constructorsThere are 2 instances of this issue:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L31
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:
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 modifiersThis is a best-practice to protect against reentrancy in other modifiers
There are 2 instances of this issue:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L279
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 factThere are 3 instances of this issue:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol#L28
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol#L26
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 declaredexternal
insteadContracts are allowed to override their parents' functions and change the visibility from
external
topublic
.There are 45 instances of this issue:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/VersionFacet.sol#L20
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L199
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/AssetFacet.sol#L66
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RelayerFacet.sol#L70
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol#L76
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/NomadFacet.sol#L11
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RoutersFacet.sol#L181
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L146
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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibDiamond.sol#L245
7.
constant
s should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 30 instances of this issue:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L408
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol#L76
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RoutersFacet.sol#L349
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L103
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L76
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L626
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/ConnextMessage.sol#L232
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibCrossDomainProperty.sol#L120
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L313
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/libraries/PromiseMessage.sol#L81
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 useimmutable
rather thanconstant
There is 1 instance of this issue:
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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/AmplificationUtils.sol#L22
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L104
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L107
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
variablesIf 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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L38
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L115
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L64
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:
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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/upgrade-initializers/DiamondInit.sol
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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L235-L244
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L625-L627
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/AssetFacet.sol#L121-L136
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/PortalFacet.sol#L71-L85
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RoutersFacet.sol#L191-L193
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BaseConnextFacet.sol#L112-L114
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/Executor.sol#L111-L113
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/LPToken.sol#L19-L21
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L317-L325
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/AssetLogic.sol#L31-L33
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L166-L184
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibCrossDomainProperty.sol#L155-L157
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L226-L230
14. Event is missing
indexed
fieldsEach
event
should use threeindexed
fields if there are three or more fieldsThere are 69 instances of this issue:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L75-L82
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/AssetFacet.sol#L27
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/PortalFacet.sol#L30
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RelayerFacet.sol#L25
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/ProposedOwnableFacet.sol#L52
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/RoutersFacet.sol#L65
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L65
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/ProposedOwnableUpgradeable.sol#L62
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/SponsorVault.sol#L73
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/AmplificationUtils.sol#L17
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/SwapUtils.sol#L25
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibDiamond.sol#L69
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/promise/PromiseRouter.sol#L78-L86
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:
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L208-L212
https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L291-L295