Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require ECDSA DKG result challenger to be an EOA #3756

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Conversation

lukasz-zimnoch
Copy link
Member

@lukasz-zimnoch lukasz-zimnoch commented Dec 14, 2023

The challengeDkgResult function uses several try-catch blocks as part of its business logic. However, the EVM has a call stack depth limit equal to 1024. A third-party contract can leverage this limitation and force the try-catch-ed calls to revert unconditionally, by using recursion and letting those calls be executed at depth 1025. In such a case, the control flow is passed to the catch clauses which may lead to undesired side effects like invalidation of a proper DKG result. To address that problem, we are adding a requirement that challengeDkgResult can only be called by an EOA. This prevents third-party contracts from calling challengeDkgResult.

Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/keep-core/actions/runs/7209109302 check.

The `challengeDkgResult` function uses several `try-catch` blocks as part
of its business logic. However, the EVM has a call stack depth limit equal to
1024. A third-party contract can leverage this limitation and force the
`try-catch`-ed calls to revert unconditionally, by using recursion and
letting those calls to be executed at depth 1025. In such a case, the
control flow is passed to the `catch` clauses which may lead to some undesired
side effects like invalidation of a proper DKG result. To address that problem,
we are adding a requirement that `challengeDkgResult` can be only called by
an EOA. This prevents third-party contracts against calling
`challengeDkgResult`.
Copy link
Member

@pdyraga pdyraga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not forget to apply the same fix to the Random Beacon contract.

@@ -800,6 +800,9 @@ contract WalletRegistry is
/// requires an extra amount of gas to be left at the end of the
/// execution.
function challengeDkgResult(DKG.Result calldata dkgResult) external {
// solhint-disable-next-line avoid-tx-origin
require(msg.sender == tx.origin, "Not EOA");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be worth adding this requirement to the function docs.

@pdyraga pdyraga enabled auto-merge December 14, 2023 12:58
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/keep-core/actions/runs/7209175432 check.

@pdyraga pdyraga merged commit 98ef3fd into main Dec 14, 2023
29 checks passed
@pdyraga pdyraga deleted the only-eoa-challenge branch December 14, 2023 13:15
@lukasz-zimnoch lukasz-zimnoch added this to the solidity/v2.0.1 milestone Dec 14, 2023
@lukasz-zimnoch
Copy link
Member Author

Let's not forget to apply the same fix to the Random Beacon contract.

Created an issue to capture that work: #3758

tomaszslabon added a commit that referenced this pull request Dec 20, 2023
The `WalletRegistry` proxy has been upgraded to the new implementation
in order to include changes from
#3756. Here we are pushing
relevant artifact updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants