// @audit --> add the checks for the length
for (uint256 i = 0; i < tokenAddresses.length; i++) {
aggregators[tokenAddresses[i]] = AggregatorV3Interface(sources[i]);
emit AggregatorUpdated(tokenAddresses[i], sources[i]);
}
}
`
due to lack of checks , whether both arrays length are equal or not, it may cause function to fail
L4. add a two step function to update the admin address
impact
Lack of two-step procedure for critical operations leaves them error-prone
if the address is incorrect, the new address will take on the functionality of the new role immediately
for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert
use a two-step procedure for all non-recoverable critical operations to prevent
irrecoverable mistakes.
L5. Lack of access modifier in initialize() function
Impact
Initialize() , Initializes important contract state that can be called by anyone. Since it lacks an access modifier, an attacker can initialize the contract before the legitimate deployer.
Proof of Concept
contracts which uses initialize(), they all lack access modifier some of the links are:
L1. Specs not according to the comment
Impact
function _originAndNonce(uint32 _origin, uint32 _nonce) internal pure returns (uint64) {
In comment it mention the return value should be
(_origin << 32) & _noncebut in function it is
(uint64(_origin) << 32) | _nonce;Proof of Concept
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/relayer-fee/RelayerFeeRouter.sol#L163
Tools Used
manual review
upadate the comment wrt current functionality, since
(_origin << 32) & _nonceit returns 0 value irrespective of any inputL2. lack of checks for the length in input parmeters
Impact
`function setAggregators(address[] calldata tokenAddresses, address[] calldata sources) external onlyAdmin {
} ` due to lack of checks , whether both arrays length are equal or not, it may cause function to fail
Proof of Concept
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L175
Tools Used
manual review
Recommended Mitigation Steps
add the checks for the length
L3. Lack of input validationn for address array
Impact
There is no checking of address array in setAggregator() , it may contain duplicate address or zero address due which function may get failed
Proof of Concept
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L176
Tools Used
manual review
Recommended Mitigation Steps
check the address before using it in loop
L4. add a two step function to update the admin address
impact
Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately
for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert
https://raw.githubusercontent.com/trailofbits/publications/master/reviews/hermez.pdf
Proof of Concept
https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L168
Tools Used
manual review
Recommended Mitigation Steps
use a two-step procedure for all non-recoverable critical operations to prevent
irrecoverable mistakes.
L5. Lack of access modifier in initialize() function
Impact
Initialize() , Initializes important contract state that can be called by anyone. Since it lacks an access modifier, an attacker can initialize the contract before the legitimate deployer.
Proof of Concept
contracts which uses initialize(), they all lack access modifier some of the links are:
bridgetoken.sol
RelayaerFeeRouter.sol
PromiseRouter.sol
Tools Used
manual review
Recommended Mitigation Steps
add access modifier in intialize function