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

fix(protocol): fix liveness bond related tokenomics issue #16684

Merged
merged 16 commits into from
Apr 8, 2024

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Apr 7, 2024

BEGIN_COMMIT_OVERRIDE
fix(protocol): return liveness bond only to assigned prover
feat(protocol): allow assigned prover to prove blocks outside proving window (liveness bond not returned)
feat(protocol): remove contestations from TransitionState and events (it's buggy)
feat(protocol): add TaikoL1.getTransition(blockId, transitionID) function
refactor(protocol): use a Local struct in LibProving to cache state variables and get around of stack-too-deep error
test(protocol): add more tests to verify bonds are handled correctly
END_COMMIT_OVERRIDE


The liveness bug

The bug was identified by(OpenZeppelin) and Code4rena.

Scenario (quoted from the bug report)

  • Henry is assigned to prove a block, and sends 250 TKO as liveness bond to the TaikoL1 contract. He correctly proves the state transition with an SGX proof, providing another 250 TKO as validity bond.
  • Mallory contests the state transition, providing 500 TKO as contest bond to the TaikoL1 contract.
  • In the same transaction, Mallory re-proves the block and confirms Henry's transition using a zk proof. In doing so, Henry earns 1/4 * contestBond = 125 TKO, and gets his original validity bond of 250 TKO back. Mallory gets 1/4 * contestBond = 125 TKO as she is the new prover. She provides a new validity bond of 500 TKO.
  • The block is verified, and Mallory as the final prover gets back half of the liveness bond as well as her validity bond, 1/2*250 + 500 = 625 TKO in total.

As a result of the above, Henry has lost his initial liveness bond but got 1/4th of the contest bond, a net loss of 125 TKO. Mallory has lost 3/4th of her contest bond but got back half of the liveness bond, a net loss of 250 TKO.

Result

  • Henry incurs a net loss of 125 TKO, losing the liveness bond but gaining a quarter of the contest bond.
  • Mallory incurs a net loss of 250 TKO, losing three-quarters of her contest bond but regaining half of the liveness bond.

Fix

The bug was in the handling of the liveness bond, which was returned to ts.prover instead of blk.assignedProver. The liveness bond was returned upon block verification, and only if the assigned prover created and used the first transition to finalize the block.

Now, the liveness bond is returned to the assigned prover immediately upon the creation of the first transition, provided it's done before the proving window expires. Otherwise, ts.livenessBond is set to 0. test_taikoL1_group_2_case_1 covers the scenario above.

@dantaik dantaik marked this pull request as ready for review April 7, 2024 12:33
@dantaik dantaik requested review from adaki2004, dionysuzx, Brechtpd and davidtaikocha and removed request for dionysuzx April 7, 2024 12:34
Copy link

openzeppelin-code bot commented Apr 7, 2024

fix(protocol): fix liveness bond related tokenomics issue

Generated at commit: 61155af7c8bbc58593366793fd1b49f8c1bd109a

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
3
39
46
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@dantaik dantaik changed the title fix(protocol): fix liveness bond return bug fix(protocol): fix liveness bond related tokenomics bug Apr 7, 2024
@dantaik dantaik changed the title fix(protocol): fix liveness bond related tokenomics bug fix(protocol): fix liveness bond related tokenomics issue Apr 7, 2024
@dantaik dantaik requested a review from Brechtpd April 8, 2024 02:15
@dantaik dantaik enabled auto-merge April 8, 2024 05:14
@dantaik dantaik requested review from Brechtpd and removed request for adaki2004 April 8, 2024 05:14
@dantaik
Copy link
Contributor Author

dantaik commented Apr 8, 2024

@Brechtpd please take another look at the last commit - suggested by David C.

@dantaik dantaik requested a review from davidtaikocha April 8, 2024 06:46
@dantaik dantaik disabled auto-merge April 8, 2024 09:42
Copy link
Contributor

@adaki2004 adaki2004 left a comment

Choose a reason for hiding this comment

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

Changes LGTM, looks it has changed a lot but local variable is causing most of the diffs.
A friendly reminder that this code (especially contestation and it's bond) part is still complex, so best would be

  • to pay high attention when modify and review
  • in case we will have reliable ZK - just switch to a one, combined tier (ZK /sg/Guardian) for the sake of quick finalization

@dantaik dantaik added this pull request to the merge queue Apr 8, 2024
Merged via the queue into main with commit 2c63cb0 Apr 8, 2024
6 checks passed
@dantaik dantaik deleted the fix_protocol_issue branch April 8, 2024 11:20
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.

4 participants