Drawing bug: test coverage + contract fix

[jaybuidl]

Thanks to @unknownunknown1 @shotaronowhere @alcercu for helping with the investigation.

Summary

The arbitrator cannot draw any juror anymore when calling KlerosCore.draw(disputeId, iterations), regardless of the number of iterations. The transaction does not revert.

Interestingly it was working fine up to disputeId 6 round 0, and started failing from round 1 onwards and disputeId 7.

Once in a while a juror still manages to get drawn successfully (example tx)

Analysis

Transaction traces

Looking at the draw transaction traces, KlerosCore goes through the max number of iterations because (almost) every drawn juror fails the DisputeKitClassic._postDrawCheck(). It turns out that this logic fails to take into account the juror's stake in the parent courts.

Why is suddenly starting to fail?

Because of a large increase of the stakes in the two subcourts (courtIds 2 and 3), relative to the stakes in the general court (courtId 1), cf. timeline below.

courtId 0           (FORK)
                      |
courtId 1          (General)
                 /           \
courtId 2  (Curate)        (English) courtId 3

In particular a farmer collected 170K PNK at address 0xFD4bA0A1C08E0aF938A2B1AC06E8937c8535eC93, then transferred it to address 0x36b6c8fe413ba60c0db27b2a67a3a0e4cd97b651 and proceeded to stake. We'll call it juror X.

Timeline

Aug-09-2023 10:01:05 PM +UTC: Juror X stakes 100K in subcourt 2

Aug-10-2023 01:25:52 AM +UTC: Drawing of voteId 0 and 1 for dispute 6 round 0

Aug-10-2023 03:07:52 AM +UTC: Juror X stakes 20K in subcourt 3

Aug-10-2023 10:37:45 AM +UTC: Drawing of voteId 2 for dispute 6 round 0, it should not have taken so long (9 hours) , there were many unsuccessful draw() attempts.

Aug-10-2023 04:33:52 PM +UTC: First successful draw of voteId 0 for dispute 6 round 1.

Until now, dispute 6 round 1 and dispute 7 round 0 could never complet their draw of 7 and 3 jurors respectively.

Reproduction

The minimal scenario to reproduce this bug is to stake in a subcourt, create a dispute in the parent court. The draw will never succeed.

kleros-v2/contracts/test/arbitration/draw.ts

Lines 188 to 203 in 7eb8634

	it("Stakes in subcourt and should draw jurors in parent court", async () => {
	const setStake = async (wallet: Wallet) => {
	await core.connect(wallet).setStake(2, ONE_THOUSAND_PNK.mul(5), { gasLimit: 5000000 });
	};
	const expectFromDraw = async (drawTx: Promise<ContractTransaction>) => {
	await expect(drawTx)
	.to.emit(core, "Draw")
	.withArgs(anyValue, 0, 0, 0)
	.to.emit(core, "Draw")
	.withArgs(anyValue, 0, 0, 1)
	.to.emit(core, "Draw")
	.withArgs(anyValue, 0, 0, 2);
	};
	await draw(setStake, "1", expectFromDraw);

Solution

Code

It consists in disabling the _postDrawCheck() for now, until the check on locked stakes can be implemented correctly.

The impact of this fix is that only the juror's overall locked PNK (total for all the courts) is checked, instead of checking also at the court level.

Additional logic has been added to gracefully handle drawing in a court where no juror has staked.

Deployment

The new DisputeKitClassic contract can be deployed, but not the new SortitionModule which is not required to fix the issue. The goal is to avoid a total redeploy which would require the jurors to stake again in the new deployment. If the SortitionModule or KlerosCore is redeployed, it is equivalent to a total redeploy.

kleros-v2/contracts/deploy/fix1148.ts

Lines 25 to 50 in ce24e73

	await deploy("DisputeKitClassic", {
	from: deployer,
	args: [deployer, AddressZero],
	log: true,
	});
	const newDisputeKit = (await ethers.getContract("DisputeKitClassic")) as DisputeKitClassic;
	await execute("DisputeKitClassic", { from: deployer, log: true }, "changeCore", klerosCore.address);
	await execute("KlerosCore", { from: deployer, log: true }, "addNewDisputeKit", newDisputeKit.address, 0);
	const oldDisputeKitId = 1;
	const newDisputeKitId = 2;
	assert(
	await klerosCore.disputeKitNodes(oldDisputeKitId).then((node) => node.disputeKit === oldDisputeKit.address),
	`wrong dispute kit id ${oldDisputeKitId}`
	);
	assert(
	await klerosCore.disputeKitNodes(newDisputeKitId).then((node) => node.disputeKit === newDisputeKit.address),
	`wrong dispute kit id ${newDisputeKitId}`
	);
	await execute("KlerosCore", { from: deployer, log: true }, "enableDisputeKits", 1, [newDisputeKitId], true); // enable the new dispute kit in court 1
	await execute("KlerosCore", { from: deployer, log: true }, "enableDisputeKits", 2, [newDisputeKitId], true); // enable the new dispute kit in court 2
	await execute("KlerosCore", { from: deployer, log: true }, "enableDisputeKits", 3, [newDisputeKitId], true); // enable the new dispute kit in court 3

deploying "DisputeKitClassic" (tx: 0x3501cc42baec8ec56bfe8d72a33814bcabb3bb58ced22ba81a270682f7fe10af)...: deployed at 0x439f92b61783A752462527f9dA9C6c6180C9253a with 3059359 gas
executing DisputeKitClassic.changeCore (tx: 0x84885ac723b669c54fb3988d195cc045d97972e7971d7e0fbf8b2f4a2798641f) ...: performed with 46126 gas
executing KlerosCore.addNewDisputeKit (tx: 0x0dbf5b18eb43f9d5871dca861286f8020c2a3339d6c8b547a0859b4c469e74ba) ...: performed with 133609 gas
executing KlerosCore.enableDisputeKits (tx: 0xf181095c90bf9577ac73a9c834be88677beaf0fe128861a1e59069c65618a8fa) ...: performed with 33913 gas
executing KlerosCore.enableDisputeKits (tx: 0x21b342299d53cfce328a1d668fbc8910fdc171b8b62bec6337da13d4a536499d) ...: performed with 53813 gas
executing KlerosCore.enableDisputeKits (tx: 0x2114c5a630e4b02d42b22fa46303d8f9d0caf3acda44190552eaacc434ae3908) ...: performed with 53813 gas

Then...

• Created a new dispute: we must specify a third 32 bytes pointing to the new DK in the extraData. For example:

extraData = "0x" +
  "0000000000000000000000000000000000000000000000000000000000000001" + // courtId 1
  "0000000000000000000000000000000000000000000000000000000000000003" + // minJurors 3
  "0000000000000000000000000000000000000000000000000000000000000002";  // disputeKitId 2

• Updated and redeployed the subgraph
• Merged to dev, then to master
• Netlify rebuilt and redeployed the frontend.
• Ran the bot again.... it has successfully drawn 3 jurors for the new disputeId 8 as expected 🥳
• Disputes 6 and 7 are toast, just ignore them.

[netlify]

✅ Deploy Preview for kleros-v2-contracts ready!

Name	Link
🔨 Latest commit	2a6024d
🔍 Latest deploy log	https://app.netlify.com/sites/kleros-v2-contracts/deploys/64d7fbce80e3600007adb142
😎 Deploy Preview	https://deploy-preview-1148--kleros-v2-contracts.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[netlify]

✅ Deploy Preview for kleros-v2 canceled.

Name	Link
🔨 Latest commit	2a6024d
🔍 Latest deploy log	https://app.netlify.com/sites/kleros-v2/deploys/64d7fbce6f238e0008d2443b

[codeclimate]

Code Climate has analyzed commit 5704cdc and detected 11 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2
Duplication	6
Style	3

View more on Code Climate.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication