Safety and Gas Patches

Compound Labs is in the process of developing patches to the Comptroller and CToken in order to improve gas efficiency and security.

This is in-progress work, but I wanted to share the changes publicly as soon as possible so that the community may audit them and leave feedback while in active development and testing.

Given the limited scope of the changes, we recommend a community bug bounty and peer review in place of a formal audit.

The changes are as follows:

Comptroller Implementation

#123: Markets with a collateral factor of 0% and borrowing paused would be considered deprecated by the Comptroller, and allowed to be completely liquidated. This allows the closure of all outstanding borrows and the removal of reserves in deprecated markets including SAI, REP, and future migrations.

#125: A gas optimization for the claimComp function, significantly improving the gas cost of claiming COMP across multiple markets at once.

cToken Implementation

#124: Modifies the seize function to transfer 2.8% of a liquidation to cToken reserves, reducing the risk of cascading liquidations that could render the protocol insolvent. With each liquidation, the protocol’s ability to recover (or utilize reserves) increases. Note: this reduces the effective liquidator incentive to ~5%, the historical baseline.

Thank you for your review and comments!

10 Likes

aaaaaaaaaaaaa left some good feedback, tether is an example of a big market with 0 collateral factor, which means we’d never be able to pause borrows (in case of emergency) w/o deprecating the whole market, which makes a lot of sense. so to deprecate markets, they suggested requiring 100% reserve factor.

implemented here Enable liquidations for all accounts in deprecated markets by maxwolff · Pull Request #123 · compound-finance/compound-protocol · GitHub

4 Likes

Hey all,

We’ve been gaining confidence on these changes and are starting a bug bounty in advance of doing a proposal.

For the next 48 hours, reported vulnerabilities in branch #127 will be eligible for a bug bounty of up to $150k according to these terms.

If you have questions feel free to reach out on the #development channel in discord.

5 Likes

Hey there,

I’m not sure why this has only 2 replies, maybe it’s already done? If not, my questions…

Where would these funds go? Could they be used to buy COMP then distribute that COMP to active suppliers and borrowers? Or maybe transfer those assets to be used for grants? Or what is the plan?

We now have a bug bounty going through Immunefi for this patch: Compound Bug Bounties | Immunefi !

See the bounty page at Immunefi for more details on accepted vulnerabilities, payout amounts, and rules of participation.

Isn’t it safer for the DAO to simply maintain a list of isDeprecated ctokens?
Either hard coded, or at a dedicated storage.
The deprecation involves a DAO vote anyway. So might as well give it the final say on what is depreciated.

I think the point is that the DAO can explicitly mark things as deprecated using this combination of parameters which should only be used for this purpose

I think this is probably a good idea. But it may be worth reviewing this from a market risk perspective. When the liquidator incentive was last at 5%, total borrowing on Compound (and in defi in general) was much lower, and gas costs have also increased substantially. Maker’s new liquidation system also clears debt much more quickly than the previous system (target of 1-2 hours vs 6 hours for old system).

All of this together increases the risk that liquidation will not be profitable with only 5% incentive, potentially allowing prices to fall further and push the market into insolvency. My inclination would be to increase the total liquidation penalty so that the liquidator incentive remains 8%, and the protocol liquidation fee is charged on top of that.

That being said, I think this change will be a net positive for Compound. More protocol revenue from riskier users (as identified by their position being liquidated) allows for charging less from safer users.

3 Likes

On first read I was thinking @maxcwolff expressed a concern about that. But now I see he actually said why it is ok.

Some empirical data suggest that the difference between 5% and 8%, currently goes to MEV anw.

But maybe th protocol could give up to 8% if the collateral is not sufficient for proper liqudiaton, and 5% otherwise.
This will guarantee 8% for the “hard liquidations”, while preventing some of the MEV for the regular ones.

1 Like

the funds would go to the collateral ctoken’s reserves directly, not into COMP. so they would serve as a safety buffer for each ctoken, subsidize rates, and could hypothetically be withdrawn by COMP holders for another purpose if they decide to

i agree, the ideal model might be something like an auction, where the market decides the discount. increasing the discount for increasingly insolvent positions is also an interesting idea.

1 Like

(English) auction worked really bad for Maker (liquidations v1). Also it will require a new intermediary state when collateral and debt are owned by some auction contract.

But a simpler solution could be something in the flavor of:
if(collateral / debt < 1.1) incentive = 1.08; else incentive = 1.05;

1 Like

Just deployed to mainnet and making a proposal soon.

New Comptroller.sol : 0x75442Ac771a7243433e033F3F8EaB2631e22938f

New cERC20.sol 0x339B2D3bf0406DF82f8fa7B0d855a3f47562d8D7

As always you can verify the contract matches the PR by checking out the branch compound/2.9, compiling, and running npx saddle match 0x339B2D3bf0406DF82f8fa7B0d855a3f47562d8D7 CErc20Delegate -n mainnet

2 Likes

Redeployed a new cERC20Delegate. The previous one did not include CCompLike vote delegation functionality. Instead of deploying a new delegate that also has vote delegation functionality, I pushed the vote delegation into Cerc20 itself and got rid of CCompLikeDelegate.sol. This way every new CToken delegate will have the ability to delegate votes, but for non-COMP-like tokens it will do nothing. The upshot is that we only need one delegate and we can avoid this mistake in the future.

You can see the diff here.

The new address is 0x3587b2F7E0E2D6166d6C14230e7Fe160252B0ba4

2 Likes

um… Do you mean protocol use cToken reserve automatically in specific liquidation status? or community multisig can decide it(use cToken reserve in liquidation process) under emergency situation?

===
Currently, I don’t have enough info for reducing liquidation incentive to support 48th proposal. as @monet-supply pointed out, I’m also concerned its impact from reducing it about 5%.

In pull #124 CToken.sol seizeInternal() it emits a Transfer event for the seized CTokens, but the amount emitted in the event is the total amount seized, not the amount transferred to the liquidator. This will confuse off-chain analytics that expect the liquidator’s CToken balance to have increased by the amount in the Transfer event.

Instead of

emit Transfer(borrower, liquidator, seizeTokens);
emit ReservesAdded(address(this), vars.protocolSeizeAmount, vars.totalReservesNew);

it could be something like:

emit Transfer(borrower, liquidator, vars.liquidatorSeizeTokens);
emit Transfer(borrower, address(this), vars.protocolSeizeTokens);
emit ReservesAdded(address(this), vars.protocolSeizeAmount, vars.totalReservesNew);

The LiquidateBorrow event might benefit from an upgrade also, as it currently shows only the total amount seized from the borrower, which is no longer the amount going to the liquidator:

event LiquidateBorrow(address liquidator, address borrower, uint repayAmount, address cTokenCollateral, uint seizeTokens);

Finally, protocolSeizeShareMantissa is an internal constant, which makes it unavailable to on-chain logic. It would be nice if this were a public variable.

4 Likes

@dakeshi the cToken reserves increase automatically; the community / COMP token-holders will continue to manage reserves as they currently do.

The liquidation incentive was historically 5%; and the markets being updated (DAI/USDT/WBTC/UNI/COMP/LINK/TUSD) are less commonly liquidated collateral than Ether, the primary collateral asset. This should provide a safer approach to experiment & improve than updating all markets.


@pyggie thank you for flagging this suggestion; we’ve reviewed it and agree that without changes, off-chain analytics could be impaired.

Given the simplicity of the change / improvement, and because we are still in the review period, we plan to:

  • Cancel the current proposal
  • Implement the split in the transfer event
  • Resubmit the proposal, and recommend a bounty be paid to pyggie

This is a great example of the review period being put to good work. Thank you!

5 Likes

Thanks for more detailed explanation @rleshner especially this part.

he markets being updated (DAI/USDT/WBTC/UNI/COMP/LINK/TUSD) are less commonly liquidated collateral than Ether, the primary collateral asset.

and I also agree to make separate proposals to help its decision and focus on specific topic.

But my question was not how to accumulate cToken reserve. I agree that increased reserve can reduce protocol risk but it is still unclear “What condition make trigger to use cToken reserve in liquidation process”. (even though it is possible to set some standard through Compound Governace) If we want to say it will be helpful to reduce risk of cascading liquidation with increased cToken reserve, I think that it should have some rule(code level or community multisig level) which cToken reserve can get involved in liquidation process.

Here’s the diff splitting the Transfer events and making the protocol share public.

Here’s the new deployment CErc20Delegate | 0xa035b9e130F2B1AedC733eEFb1C67Ba4c503491F

@pyggie decided not to modify LiquidateBorrow event. It is emitted inside the borrowed token, not the collateral token which gets seized, the borrowed token would have to know if the collateral token is going to have a protocol share, and that gets complicated.

1 Like