Skip to content

Commit

Permalink
Require ECDSA DKG result challenger to be an EOA (#3756)
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
pdyraga authored Dec 14, 2023
2 parents 1d813c5 + d8c2471 commit 98ef3fd
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 0 deletions.
3 changes: 3 additions & 0 deletions solidity/ecdsa/contracts/WalletRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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");

(
bytes32 maliciousDkgResultHash,
uint32 maliciousDkgResultSubmitterId
Expand Down
18 changes: 18 additions & 0 deletions solidity/ecdsa/contracts/test/DkgChallenger.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// SPDX-License-Identifier: GPL-3.0-only

pragma solidity 0.8.17;

import "../WalletRegistry.sol";
import "../libraries/EcdsaDkg.sol";

contract DkgChallenger {
WalletRegistry internal walletRegistry;

constructor(WalletRegistry _walletRegistry) {
walletRegistry = _walletRegistry;
}

function challengeDkgResult(EcdsaDkg.Result calldata dkgResult) external {
walletRegistry.challengeDkgResult(dkgResult);
}
}
22 changes: 22 additions & 0 deletions solidity/ecdsa/test/WalletRegistry.WalletCreation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {
WalletRegistryStub,
TokenStaking,
IRandomBeacon,
DkgChallenger,
} from "../typechain"
import type { DkgResult, DkgResultSubmittedEventArgs } from "./utils/dkg"
import type { Operator } from "./utils/operators"
Expand Down Expand Up @@ -2244,6 +2245,27 @@ describe("WalletRegistry - Wallet Creation", async () => {
})

describe("challengeDkgResult", async () => {
context("with caller being a contract", async () => {
let dkgChallenger: DkgChallenger

before("request new wallet", async () => {
await createSnapshot()

const DkgChallenger = await ethers.getContractFactory("DkgChallenger")
dkgChallenger = await DkgChallenger.deploy(walletRegistry.address)
})

after(async () => {
await restoreSnapshot()
})

it("should revert", async () => {
await expect(
dkgChallenger.challengeDkgResult(stubDkgResult)
).to.be.revertedWith("Not EOA")
})
})

context("with no wallets registered", async () => {
it("should revert with 'Current state is not CHALLENGE'", async () => {
await expect(
Expand Down

0 comments on commit 98ef3fd

Please sign in to comment.