-
Notifications
You must be signed in to change notification settings - Fork 75
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
Conversation
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`.
7eddc75
to
d8c2471
Compare
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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.
Solidity API documentation preview available in the artifacts of the https://github.com/keep-network/keep-core/actions/runs/7209175432 check. |
Created an issue to capture that work: #3758 |
The `WalletRegistry` proxy has been upgraded to the new implementation in order to include changes from #3756. Here we are pushing relevant artifact updates.
The
challengeDkgResult
function uses severaltry-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 thetry-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 thecatch
clauses which may lead to undesired side effects like invalidation of a proper DKG result. To address that problem, we are adding a requirement thatchallengeDkgResult
can only be called by an EOA. This prevents third-party contracts from callingchallengeDkgResult
.