Connext Amarok contest - JMukesh's results

L1. Specs not according to the comment

Impact

function _originAndNonce(uint32 _origin, uint32 _nonce) internal pure returns (uint64) {

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

In comment it mention the return value should be (_origin << 32) & _nonce

but 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) & _nonce it returns 0 value irrespective of any input

L2. lack of checks for the length in input parmeters

Impact

`function setAggregators(address[] calldata tokenAddresses, address[] calldata sources) external onlyAdmin {

// @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

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