Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(flash-swap): support liquidating vaults with underlying as collateral #64

Merged
merged 16 commits into from
Nov 5, 2021

Conversation

scorpion9979
Copy link
Contributor

@scorpion9979 scorpion9979 commented Nov 3, 2021

Regarding liquidating USDC collateral, the following modifications needed to be applied to the flash-swap contract:

  1. The bot is not able to access a USDC-USDC trading pair, and so for this special case we are assuming the use of any pair that has USDC on one side. In real life, the pair would preferably be the one with the largest liquidity out of all pairs containing USDC on one side.
  2. Since we're only going to be using one side of the trading pair, we need to repay the borrowed USDC in USDC + 0.3% Uniswap v2 fee. This required modifying the getRepayCollateralAmount(...) function in flash-swap to enable different calculation for this special case.
  3. We also needed to do something about the msg.sender verification in flash-swap, as it previously required the msg sender address to be equal to the pair address of underlying and collateral computed using pairFor(...). Since it's not possible to have a USDC-USDC pair, we needed to constrain this check to the case where the underlying token is not the same as the collateral to be liquidated.
  4. The liquidated collateral is not going to be enough to repay the 0.3% Uniswap fee. A modification was needed to enable the liquidation bot wallet to repay the fee from its own balance if necessary. The flash swap contract expects there would be no profit from liquidations, as the USDC collateralization requirement is 100%.

@scorpion9979 scorpion9979 force-pushed the scorpion9979-underlying-collateral-alt branch from 6625f4a to 59db094 Compare November 3, 2021 19:24
scorpion9979 and others added 5 commits November 3, 2021 23:07
…apV2"

chore: update flash contract deployers to use the latest contract names
chore(tasks): update flash contract deployers to use the latest contract names
refactor(flash-swap): "HifiFlashUniswapV2Underlying" into "UnderlyingFlashUniswapV2"
refactor(flash-swap): "HifiFlashUniswapV2Utils" into "FlashUtils"
test(flash-swap): update tests to use the latest contract names
…ngFlashUniswapV2"

chore(amm,flash-swap,proxy-target): use "old" and "new" prefixes instead of "pre" and "post"
chore(flash-swap): better explain the role of subsidizer in the "uniswapV2Call" function
refactor(errors): "FlashBorrowWrongToken" to "FlashBorrowOtherToken"
refactor(flash-swap): "bot" to "subsidizer"
refactor(flash-swap): "collateral" to "otherToken"
refactor(flash-swap): "collateralRepayAmount" to "repayCollateralAmount"
refactor(flash-swap): delete "collateralAmount" arg in "FlashBorrowCollateral" custom err
refactor(flash-swap): delete superfluous "vars.swapToken" var
refactor(flash-swap): "FlashLiquidateBorrow" event to "FlashSwapCollateralAndLiquidateBorrow"
refactor(flash-swap): "FlashLiquidateBorrow" event to "FlashSwapUnderlyingAndLiquidateBorrow"
refactor(flash-swap): "getCollateralAndUnderlyingAmount" to "getOtherTokenAndUnderlyingAmount"
refactor(flash-swap): "getRepayCollateralAmount" to "getRepayUnderlyingAmount"
refactor(flash-swap): "overshootCollateralAmount" to "shortfallUnderlyingAmount"
refactor(flash-swap): import "IHToken" in "FlashUtils.sol"
test(flash-swap): order vars alphabetically
test(flash-swap): rename "usdcRepayFeeAmount" var to "usdcFeeAmount"
…2" tests

test(flash-swap): import "LIQUIDATION_INCENTIVES" to set liquidation incentives for USDC/ WBTC
@PaulRBerg
Copy link
Contributor

PaulRBerg commented Nov 4, 2021

I refactored pretty much everything. For a detailed log, see the commit messages. Here's the high level:

  • I renamed HifiFlashUniswapV2 to CollateralFlashUniswapV2 and HifiFlashUniswapV2Underlying to UnderlyingFlashUniswapV2. The former names were too verbose, and they lacked symmetry - why does a contract have a suffix but the other sounds like a generic flash swap implementation? With the updated naming, it's much clearer what is the role played by each contract. This came at the cost of removing the "Hifi" prefix, but this contract is meant to be used internally, so it shouldn't affect our business image on Etherscan.
  • I moved the common logic between the two different flash swap contracts in a new library called FlashUtils.
  • I pruned the code in UnderlyingFlashUniswapV2 such that there is no reference to the word "collateral" anymore. Even if the underlying is used as collateral, it is confusing to assign the role of the "underlying" to the "collateral" during the function execution. That sort of goes against the principle of Static single assignment form.
  • Polished the variable names such that they all follow the structure "scopeTokenAmount" instead of "tokenScopeAmount" (e.g. I changed usdcDepositAmount to depositUsdcAmount). This is for monorepo-wide consistency purposes.
  • Renamed the variable bot to subsidizer. The contract need not be called by a bot. Any accounts with the ability to subsidize the transaction cost would do.

You can add your review @scorpion9979 and then I will merge the PR.

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Nov 4, 2021

After another review, I noticed three other issues:

  1. Many tests written for UnderlyingFlashUniswapV2 are pointless. All the tests whose beforeEach block manipulate the WBTC price (probably copy-pasted from the other contract's test), but in this case it is not possible to have a liquidity shortfall when the borrow is backed by the underlying itself, and there is no swap made, so the UniswapV2Pair contract does NOT give any price.
  2. The tests for UnderlyingFlashUniswapV2 should have accounted for the time-contingent nature of underlying-backed vaults.
  3. The implementation is incomplete: the "else" logical branch of the if block below should have been handled:
if (vars.repayUnderlyingAmount > vars.seizedUnderlyingAmount)

Any surplus of underlying should be relayed to the caller, just like we're doing in CollateralFlashUniswapV2.

Already started working on fixes.

…nderlyingAmount"

docs(flash-swap): better formulate the NatSpec comments for "getRepayUnderlyingAmount"
test(flash-swap): add time-bound tests for "UnderlyingFlashUniswapV2"
test(flash-swap): delete price-related tests for "UnderlyingFlashUniswapV2"
test(flash-swap): refer to the "pair contract" as "UniswapV2Pair contract" in descriptors
test(flash-swap): rename "bumpPoolReserves" to "increasePoolReserves"
test(flash-swap): rename "collateralAmount" to "swapCollateralAmount"
test(flash-swap): rename "repayHUsdcAmount" to "repayAmount"
test(flash-swap): rename "underlyingAmount" to "swapUnderlyingAmount"
@PaulRBerg
Copy link
Contributor

PaulRBerg commented Nov 4, 2021

This was a hairy refactor ... I made progress with the latest commit, but the following test cases are still not written:

  • context("when the repay underlying amount is less than the seized underlying amount", function () {})
  • context("when the repay underlying amount is equal to the seized underlying amount", function () {})

I'll handle them tomorrow.

docs(flash-swap): rewrite the @notice for "liquidateBorrowInternal" function
refactor(flash-swap): emit "profitUnderlyingAmount" and "subsidizedUnderlyingAmont" in event
refactor(flash-swap): rename "shortfallUnderlyingAmount" to "subsidizedUnderlyingAmount"
refactor(flash-swap): rename "surplusUnderlyingAmount" to "profitUnderlyingAmount"
test(flash-swap): erc-20 approve to "MaxUint256"
test(flash-swap): refer to USDC as "underlying" in variable names
test(flash-swap): rename "repayAmount" to "repayHTokenAmount"
test(flash-swap): when the repay underlying amount is less than the seized underlying amount
@scorpion9979 scorpion9979 force-pushed the scorpion9979-underlying-collateral-alt branch from 6ebee5b to 65a497b Compare November 5, 2021 13:11
test(flash-swap): remove redundant debug console logs
@PaulRBerg PaulRBerg force-pushed the scorpion9979-underlying-collateral-alt branch from 65a497b to 190e0ec Compare November 5, 2021 15:41
@PaulRBerg PaulRBerg force-pushed the scorpion9979-underlying-collateral-alt branch from 247feb2 to 5ced780 Compare November 5, 2021 15:47
@PaulRBerg PaulRBerg merged commit 3308a45 into main Nov 5, 2021
@scorpion9979 scorpion9979 deleted the scorpion9979-underlying-collateral-alt branch November 6, 2021 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants