Analysis of Proposal 64

Analysis of Proposal 64

On October 1, Tyler L. created Proposal 64, an upgrade to the Compound Comptroller which patches the bug introduced in Proposal 62, which separated COMP distribution speeds between suppliers and borrowers and contained an unintentional bug. This new proposal a) patches the bug, preventing new users from being granted excessive COMP rewards, and b) locks COMP reward transfers for users who may have been affected by the bug. Further analysis is below on the means of the fix and which accounts will be prevented from COMP reward withdrawals.

Note: there will need to be another patch, after Proposal 64, to fully fix the issue and allow the rest of the accounts to continue standard accrual and transfers of COMP rewards. In the best case scenario, all accounts should continue to receive all rewards, past, present and future if some future (yet unbuilt) proposal is correctly applied.

Note: this patch (nor any other) has suggested recovering excess COMP that was given out to accounts. Instead, it aims to fix the accounting and return to proper accruals.

Effects of Proposal

Proposal 64 includes two adjustments to the Compound Comptroller functionality.

First, it changes line 1217 of distributeSupplierComp as follows:

- if (supplierIndex == 0 && supplyIndex > compInitialIndex) {
+ if (supplierIndex == 0 && supplyIndex >= compInitialIndex) {

and line 1256 of distributeBorrowerComp similarly:

- if (borrowerIndex == 0 && borrowIndex > compInitialIndex) {
+ if (borrowerIndex == 0 && borrowIndex >= compInitialIndex) {

Each missing equality check above excludes an entire set of uninitialized accounts from the conditional body. This in turn prevents those accounts from becoming initialized, since they incorrectly accrue COMP from an under-initialized position.

These changes prevent the bug from occuring for future users. Specifically: any account that would have been given excess COMP per the issue identified from Proposal 62, after this patch takes effect, will not be given excess COMP when later claiming COMP (or interacting with an affected market). Note: this does not undo the excess COMP that was accrued or distributed to accounts before this patch takes effect. Overall, this patch should return the Comptroller system to its normal path, except there are several accounts that did trigger the bug that will need to be properly accounted for before they can safely withdraw any future COMP rewards.

Thus, secondly, this patch includes the following patch in the beginning of grantCompInternal on line 1350:

+ for (uint i = 0; i < allMarkets.length; ++i) {
+   address market = address(allMarkets[i]);
+
+   bool noOriginalSpeed = compBorrowSpeeds[market] == 0;
+   bool invalidSupply = noOriginalSpeed && compSupplierIndex[market][user] > 0;
+   bool invalidBorrow = noOriginalSpeed && compBorrowerIndex[market][user] > 0;
+
+   if (invalidSupply || invalidBorrow) {
+     return amount;
+   }
+ }

The accounts that tripped the bug would have been given a non-zero index in the markets which did not have COMP rewards associated with them. For users that did not trip the bug, they would have and retain a zero index in such markets. Note: we use the old storage value to check if they had not been given a COMP speed. If this is true on either the supply or borrow side, we simply return with a no-op (that is: we do not transfer COMP and we return the COMP accrued is the same as the input value).

This snippet will, in effect, prevent an account which may have received excess COMP from claiming COMP from the protocol. It will not revert, but simply will not transfer out any COMP until a future patch that would ensure proper accounting for all such affected accounts.

Note: The check is overly broad and will prevent some users who have not actually hit the bug from claiming COMP as well – namely anyone who has interacted with a market which does not currently have a COMP (borrow) speed (cREP, cWBTC, cSAI, cAAVE, cYFI, cTUSD, cMKR, cSUSHI)

Note: In order for this patch to remain effective, no changes to COMP speeds should be made until claiming is unblocked for all users in the future patch.

The patch looks for accounts which may have been granted excess COMP, but it’s not obvious from the on-chain system if those accounts actually did since that would require knowing if those accounts previously had a supply or borrow balance, but that balance may have since changed. As such, there will need to be a follow-up patch to allow the rest of accounts in this category from withdrawing. This precaution will continue to affect new accounts that claim COMP from afflicted markets, even after Prop 64 is applied.

Review

From a variety of analyses, it does appear that the patch works as intended. That is: new accounts will no longer trip the bug, and some subset of accounts deemed definitely safe will be able to continue standard COMP claims. There has been work on creating scenarios and web3-forks that show the intended result. Additionally, even if there were accounts that were false-negatives from the transfer function, this would be no worse than the functionality that is present today. But it is likely that this patch does fix the bug and prevent any excess withdrawals.

The biggest downside of this patch is that it continues to effectively lock several accounts from honest COMP reward withdrawals, even if those accounts first interact with the affected markets after Prop 64 would go live. Thus, it will be required that a new patch be created soon afterwards to ensure that all accounts become unblocked. This patch is thus a temporary patch. That said, it is one that allows the protocol to return to normal for a large number of users.

Other Factors

As opposed to Prop 63, this patch should not negatively affect any protocol integrations. Specifically, the worst situation for an integration is that it does not receive the COMP rewards it believes it is expected to receive. This situation is no different than if the Comptroller did not have the COMP balance to complete the transfer and should have been assumed when building product integrations. As opposed to Prop 63, this patch does not revert for any user.

This patch has not been audited officially and should be reviewed thoroughly by the community. The intent of the patch is to do no harm, but the pros and cons of the change should be thoroughly reviewed before any community member makes a vote on this proposal.

Note: it is possible to pass and executed Proposal 63 and then Proposal 64. This should be evaluated a potential option, as well.

References

15 Likes

Great analysis.

I’m about to start working on the code for proposal 65. Here’s what this future proposal will do:

  1. Correct the accrued COMP amounts for the affected users
  2. Remove the grantCompInternal changes to allow all users to be able to withdraw their accrued COMP

A key requirement before we can submit proposal 65 is for proposal 64 to be executed. We need it to be executed so that we can generate a definite list of affected users.

It is my intention to make proposal 65 as simple as possible to minimize the risk of adding additional bugs as well as restoring Compound to its fully working state as soon as possible.

Some members have talked about using merkel proofs as a way to fix the bad accrual amounts for affected users. While this would be a great feature to have, it would probably take a month or two of review and testing to ensure that it works well - far too long in my opinion to get Compound back on track.

So, therefore, it is my intention to use off-chain analysis to generate a list of users and the amounts they accrued incorrectly, and this list will be used in a one-off function to fix the bad accrual amounts. It’ll be up to the community to verify that this list is 100% correct and also to vote on passing it. Tools will be provided so that anyone can verify this list.

Edit:

4 Likes

The script to compute the list of users who have over-accrued COMP along with the amount of COMP they have over-accrued has been published and is available for anyone to run.

3 Likes

for those of us who just uses comp and not in the dev details. can you please tell us the date when everything will be back to normal where comp distribution will resume for the various coins. thanks

also just a general comment it’s amazing we have this meshed up process that takes weeks to resolve critical fix for a system hosting billions of dollars, god help us if the next bug affects user funds. Instead of raiding the comp coffer wasting them to reward legacy users or marketing bs, how about focus funding and hiring a real dev team dedicated to maintaining comp as their real full time job.

nuts