search

compound-finance / compound-protocol

The Compound On-Chain Protocol
https://compound.finance/developers
BSD 3-Clause "New" or "Revised" License
1.89k stars 1.21k forks source link

Fix COMP rewards bugs #148

Closed TylerEther closed 3 years ago

TylerEther commented 3 years ago

This PR fixes two separate but related bugs involving COMP accrual.

Note: Test cases need to be written to ensure both of these bugs never occur again (should be put in spec/scenario/Flywheel/Flywheel.scen).

jflatow commented 3 years ago

Also, this reminds me of the initialization issue https://github.com/compound-finance/compound-protocol/issues/40, which I remember was particularly tricky to have avoided but don't recall all the reasons why. I think whoever fixes this should have an understanding of what went wrong then too, and how this is different, and what changed in this code in between.

TylerEther commented 3 years ago

Good job finding a bug in the Gauntlet changes. Sadly, these changes make a complicated function even more complicated. I think there is a bunch more work to be done to show that these are the correct set of fixes (testing wise), and we should consider if there is something that can be done to simplify this code while also fixing it (otherwise we've introduced a bunch of new untested branches and my own faith in this code continues to drop). Also I think whoever does this work properly should receive a bunch of COMP.

Thanks for the reply. I agree that additional test cases should be written to ensure that the problem is entirely fixed and so it doesn't happen again. elee said he would help in this regard.

I disagree with simplifying the code at the same time. The more changes, the more likely that new bugs will be introduced. I think the bug should be fixed first with a minimal amount of changes, then any simplifications can be done at a later time with even more review and testing.

TylerEther commented 3 years ago

Also, this reminds me of the initialization issue #40, which I remember was particularly tricky to have avoided but don't recall all the reasons why. I think whoever fixes this should have an understanding of what went wrong then too, and how this is different, and what changed in this code in between.

This patch fixes the initialization issue. Getty, elee, and I worked together on this and we all have an understanding of what went wrong, why it happened, and how these changes fix the issue. We've written a report here: Comptroller compSpeed bug.

jflatow commented 3 years ago

Also, this reminds me of the initialization issue #40, which I remember was particularly tricky to have avoided but don't recall all the reasons why. I think whoever fixes this should have an understanding of what went wrong then too, and how this is different, and what changed in this code in between.

This patch fixes the initialization issue. Getty, elee, and I worked together on this and we all have an understanding of what went wrong, why it happened, and how these changes fix the issue. We've written a report here: Comptroller compSpeed bug.

There were changes to this code since issue #40 was initially filed, I haven't seen any discussion about that?

elee1766 commented 3 years ago

Good job finding a bug in the Gauntlet changes. Sadly, these changes make a complicated function even more complicated. I think there is a bunch more work to be done to show that these are the correct set of fixes (testing wise), and we should consider if there is something that can be done to simplify this code while also fixing it (otherwise we've introduced a bunch of new untested branches and my own faith in this code continues to drop). Also I think whoever does this work properly should receive a bunch of COMP.

i mentioned to trilez in our gc that the contract looked a bit messier than it should, but we felt it was best to not touch the contracts too much for the purposes of patching the error and moving forward as worse is better for now.

i'm more than happy to go through and rewrite all this logic in a cleaner fashion, I personally feel it would be good in the long run to fix up the code instead of trying to throw water off board with buckets.

jflatow commented 3 years ago

Good job finding a bug in the Gauntlet changes. Sadly, these changes make a complicated function even more complicated. I think there is a bunch more work to be done to show that these are the correct set of fixes (testing wise), and we should consider if there is something that can be done to simplify this code while also fixing it (otherwise we've introduced a bunch of new untested branches and my own faith in this code continues to drop). Also I think whoever does this work properly should receive a bunch of COMP.

i mentioned to trilez in our gc that the contract looked a bit messier than it should, but we felt it was best to not touch the contracts too much for the purposes of patching the error and moving forward as worse is better for now.

i'm more than happy to go through and rewrite all this logic in a cleaner fashion, I personally feel it would be good in the long run to fix up the code instead of trying to throw water off board with buckets.

Yeah I kinda agree, I'm just concerned there's actually some more subtle issues here that aren't being addressed. There are a lot of branches now so its really hard to follow and I don't believe we have coverage of all the cases that can occur anymore