BUG: fix credit unlock rate bug

[thetechnocratic]

No description provided.

[thetechnocratic]

Report ID
#12058
Target
https://etherscan.io/address/0x062Bf725dC4cDF947aa79Ca2aaCCD4F385b13b5c

Report type
Smart Contract
Impacts
Temporary freezing of funds for at least 1 day
Has PoC?
Yes
Details
Description
Bug Description
When configureCreditUnlockRate is called to increase the number of blocks needed for full unlock (decrease rate), it is effective immediately. At the same time, the proportion of distributedCredit and pendingCredit is not altered.

If distributedCredit is large at that moment (just before harvest, lots unlocked), it will be larger than what it would have potentially been under the new unlock regime (as if we "took the unlocking back"). This has the effect of producing an underflow in _calculateUnlockedCredit (https://github.com/alchemix-finance/v2-foundry/blob/master/src/AlchemistV2.sol#L1629), since pendingCredit * percentUnlocked / FIXED_POINT_SCALAR) < distributedCredit.

As a consequence, every call to this function, its callers: _distributeCredit, _distributeUnlockedCredit, _calculateUnrealizedDebt, accounts, bubbling up to almost the entire contract API including harvest, poke, burn, repay, liquidate, donate, deposit, withdraw, mint, is going to revert.

Impact
Assuming the pessimistic timing (just before harvest) but realistic configureCreditUnlockRate change (from 24h to 48h), the contract becomes unusable for about 24 hours until pendingCredit * percentUnlocked catches up. This leads to freezing of funds (not being able to mint or withdraw), inability to operate (harvest, liquidate, burn), read contract state by other contracts (accounts), as well as some PR damage if the outage is not communicated prior.

One can also imagine that this condition is used to maliciously grief the contract, by front running the configureCreditUnlockRate transaction with a dust burn transaction which will bump distributedCredit to its maximum possible value (as demonstrated in the PoC). Even with the "soft" recommendation in place (only send configureCreditUnlockRate after harvest), it is theoretically possible that the signed configureCreditUnlockRate is going to be maliciously withheld until enough time had passed to damage the contract.

Recommendation
Only send configureCreditUnlockRate transactions right after harvest, ensuring that it doesn't get delayed. Ideally, one could deploy a whitelisted contract allowed to perform these two activities atomically.

Long term, configureCreditUnlockRate could have a break-in period, where it becomes effective only after the next harvest. Amending distributedCredit at the moment of this call is also an option, as is requiring configureCreditUnlockRate to only succeed if pendingCredit * percentUnlocked / FIXED_POINT_SCALAR) >= distributedCredit. Yet another option is to catch this condition in _calculateUnlockedCredit and return 0.

Proof of concept
In the "poke" describe section, add the following test:

it("demonstrates how reconfiguring of the creditUnlockRate breaks the contract for some time", async () => {
  // Assume this is the usual unlock rate
  await alchemist
    .connect(admin)
    .configureCreditUnlockRate(yieldToken.address, 600);

  // some setup...
  const totalSupply = await yieldToken.totalSupply();
  const minAmtOut = profit.mul(depositAmount).div(totalSupply).mul(1).div(10000); // 1bps slippage allowed
  const mintAmount = parseUnits("2000", "ether");
  await alchemist.mint(mintAmount, wallet.address);
  await alchemist.accounts(wallet.address);

  // 1. Harvest some, so that there is `pendingCredit`, all of it is locked.
  await alchemist.connect(admin).harvest(yieldToken.address, minAmtOut);
  
  // 2. Wait some time, so that the next unlocking is substantial
  await mineBlocks(waffle.provider, 500);

  // 3. Attack! this transaction can be maliciously placed in just before the next, but it
  // can also happen due to normal operation of the contract.
  // The way it works, is that it calls `_distributeUnlockedCredit`, which advances
  // `distributedCredit` by a lot (almost all of the 600 unlock blocks had passed).
  const burnAmount = parseUnits("10", "wei");
  await debtToken.approve(alchemist.address, burnAmount);
  await alchemist.burn(burnAmount, wallet.address);

  // 4. Trap. We're doubling the number of blocks to fully unlock the credit. From now on
  // `(pendingCredit * percentUnlocked / FIXED_POINT_SCALAR) - distributedCredit` from
  // https://github.com/alchemix-finance/v2-foundry/blob/master/src/AlchemistV2.sol#L1629
  // is broken because it's negative and implicit SafeMath reverts.
  await alchemist
    .connect(admin)
    .configureCreditUnlockRate(yieldToken.address, 1200);

  // 5. From now on, most of the contract is out of service, until enough blocks are mined for
  // `pendingCredit * percentUnlocked` to catch up with `distributedCredit`
  // Anything attempting to `_distributeUnlockedCredit` will fail
  await debtToken.approve(alchemist.address, burnAmount);
  // The next line will panic with `Arithmetic operation underflowed or overflowed outside of an unchecked block`
  await alchemist.burn(burnAmount, wallet.address);
});

In my case it was line 855 of https://github.com/alchemix-finance/v2-foundry/blob/a5bfebfc260fd936762c791bd4367f6ec8c7f4f4/test/AlchemistV2.spec.ts

Run tests with npx hardhat test

Observe Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)