search

code-423n4 / 2022-06-connext-findings

1 stars 0 forks source link

Low level calls with solidity version 0.8.14 can result in optimiser bug. #260

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/helpers/TokenRegistry.sol#L2 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/LibDiamond.sol#L2 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/core/connext/libraries/LibConnextStorage.sol#L2 https://github.com/code-423n4/2022-06-connext/blob/b4532655071566b33c41eac46e75be29b4a381ed/contracts/contracts/test/Connext.sol#L2

Vulnerability details

Impact

The protocol is using low level calls with solidity version 0.8.14 which can result in optimizer bug.

Proof of Concept

See POC from Certora

Recommended Mitigation Steps

Consider upgrading to solidity 0.8.15

ecmendenhall commented 2 years ago

This is indeed a gnarly optimizer bug in Solidity 0.8.13 and 0.8.14, but it only occurs under very specific conditions. In order to trigger the bug, the legacy optimizer must be enabled (true for the current project configuration), and the affected assembly blocks must not refer to any local Solidity variables.

The second condition does not appear true for the examples here: every inline assembly block in the linked files for this finding references an outside variable.

ecmendenhall commented 2 years ago

Trying to keep my comments factual, but I would also note that 1) I think this is still worth reporting, and 2) I have reported this myself as an informational finding in other contests, even when there was no evidence of vulnerable assembly blocks.

LayneHaber commented 2 years ago

Agree that this is worth reporting, and we will adopt the suggestions. Think we can lower the severity since it doesn't seem to fit all of the conditions. Perhaps this is more of a QA issue?

LayneHaber commented 2 years ago

https://github.com/connext/nxtp/pull/1450/commits/900de7a6a85c7983ac3b099c880fb0487e4f0f09

0xleastwood commented 1 year ago

This bug is definitely an interesting one to breakdown. It only affects self-contained assembly code when the yul optimiser is enabled. As a result, memory operations are removed from the block, leading to unexpected handling of memory later in the function. This can introduce some unintended behaviour later on, but this does not affect the connext codebase. Downgrading to QA and merging with #263.

0xleastwood commented 1 year ago

Here is some more context https://blog.soliditylang.org/2022/06/15/inline-assembly-memory-side-effects-bug/