code423n4 commented 2 years ago

Equality check with different uint sizes can cause failures

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L283 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L146 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L41

Vulnerability details

Impact

In BridgeFacet.sol the xcall() function checks if _args.params.originDomain != s.domain and it will revert if these are not equal. The problem is that the originDomain is a uint32 and the s.domain is a uint256. This means that if the s.domain number is ever larger than the max value for a uint32 value, this function will fail every time because the originDomain could never reach a large enough number.

Proof of Concept

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L283

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L146

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L41

Tools Used

Manual code review

Recommended Mitigation Steps

The _args.params.originDomain and the s.domain should both be the same uint type. They both should be either a uint32 or uint256 to avoid any possible failures due to numbers not being large enough.

jakekidd commented 2 years ago

This is a QA issue - basically, it's possible for Owner to produce a bad configuration, using an invalid domain ID (official domain IDs are listed by Nomad and all conform to being uint32).

jakekidd commented 2 years ago

Resolved by https://github.com/connext/nxtp/commit/912830f2061403d53142ced7184ddf058e0aa8eb

0xleastwood commented 1 year ago

I mean this isn't really an issue but a worthwhile QA for consistent equality checking.