Skip to content

fix(dec-20-audit): [H01] Bond penalty may not apply#2329

Merged
mrice32 merged 2 commits intoUMAprotocol:masterfrom
mrice32:h01
Dec 23, 2020
Merged

fix(dec-20-audit): [H01] Bond penalty may not apply#2329
mrice32 merged 2 commits intoUMAprotocol:masterfrom
mrice32:h01

Conversation

@mrice32
Copy link
Copy Markdown
Member

@mrice32 mrice32 commented Dec 21, 2020

Motivation

The optimistic oracle is designed so that in the event of a dispute, the incorrect party pays the bond penalty to the vindicated party, as determined by the DVM. However, if the proposer and disputer are the same entity, this transfer has no effect. The only remaining deterrent is the DVM fee. Since the bond size is specifically chosen to dissuade attackers from submitting the wrong price and delaying resolution, the ability to nullify the bond penalty undermines the economic reasoning. Moreover, if the reward exceeds the DVM fee, the attacker may actually be positively rewarded for delaying the resolution.

This attack does not apply to contracts deployed from the Perpetual Multiparty template, because they disregard disputed price requests. Nevertheless, it does limit the simplicity and applicability of the optimistic oracle. Consider burning some or all of the bond penalty and ensuring the reward is not high enough to compensate.

Summary

As suggested above, half of the bond is paid to the store in order to "burn" it. This makes the loss for someone who intentionally delays a request by proposing and disputing themselves proportional to the bond.

Issue(s)

N/A

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
@mrice32 mrice32 added the dec-2020-audit Fixes for the perpetual, optimistic oracle, ancillary DVM data and financial product library audit. label Dec 21, 2020
@mrice32 mrice32 requested review from a team, chrismaree, daywiss, nicholaspai and pemulis and removed request for a team and daywiss December 21, 2020 22:50
uint256 bond = request.bond;

// Unburned portion of the loser's bond = floor(bond / 2)
uint256 unburnedBond = bond.div(2);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any way to link unburnedBond and burnedBond via state variables? It's not very readable how burnedBond + unburnedBond = bond.

However, I don't really like the idea of storing burnedBond on-chain because it contributes bytecode and gas to store a variable that isn't that important to the user, so I understand why you did it this way.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe we could use a helper method to standardize the computation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, we could return burnedBond + unburnedBond + an assertion that their sum equals bond which should help reduce some of the testing code

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added a function to compute the burned bond and compute unburned bond by doing bond - burned bond, which should guarantee burned bond + unburned bond = bond.

await verifyBalanceSum(optimisticOracle.address, reward, totalCustomBond);
});

it("Burned bond rounding", async function() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 for testing rounding

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
@mrice32 mrice32 requested a review from nicholaspai December 23, 2020 03:13
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 93.261% when pulling 64714a7 on mrice32:h01 into a5070ae on UMAprotocol:master.

Copy link
Copy Markdown
Member

@chrismaree chrismaree left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

Thanks for adding the internal method I think this makes it more readable and easier to change in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dec-2020-audit Fixes for the perpetual, optimistic oracle, ancillary DVM data and financial product library audit.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants