werg
commented
3 years ago I believe the issue above is referring to the "overcollateralized borrow" functionality, because there is talk of withdrawing. In any case of withdrawal (whether immediately or not) it is not the liquidationThresholdPercent which governs how much a user may withdraw. Rather, we check whether the account has positive balance (i.e. the value of assets exceeds the loan).
In the current system, at 3x possible leverage level, users can withdraw maximally 66% of the face value (to the system) they deposited. If a user deposited collateral that had dropped in price by 10% it would allow them to withdraw around 73% of the real value. -- Not undercollateralized. The price of an asset would have to have dropped by 33%, without the system catching on, for Alice to break even (without considering gas cost).
Cross margin trading will only be available for a select set of tokens with high enough trading volume. Anyone will be able to update the price if our exponential weighted average is out of date. Nevertheless, risk remains as in any lending system.
- We will consider adding an additional buffer around immediate withdrawals
- If staleness becomes an issue the protocol can institute rewards for updating the price
Handle
jvaqa
Vulnerability details
Users Can Drain Funds From MarginSwap By Making Undercollateralized Borrows If The Price Of A Token Has Moved More Than 10% Since The Last MarginSwap Borrow/Liquidation Involving Accounts Holding That Token.
Impact
MarginSwap's internal price oracle is only updated for a particular token if a borrow or liquidation is attempted for an account that is lending/borrowing that particular token. For a less popular token, the price could move quite a bit without any borrow or liquidation being called on any account lending/borrowing that token, especially if MarginSwap does not end up being wildly popular, or if it supports lesser known assets. If the Uniswap price has moved more than 10% (liquidationThresholdPercent - 100) without a borrow or liquidation on an account lending/borrowing that particular token occurring on MarginSwap, then Alice can make undercollateralized loans, leaving behind her collateral and draining funds from the contract.
Proof of Concept
(1) Alice waits for the Uniswap price for any token to move more than 10% (liquidationThresholdPercent - 100) without a borrow or liquidation occurring for any account lending/borrowing that token occurring on MarginSwap. (2) When this condition is satisfied, Alice can loop the following actions: (2.1) If the price has fallen, Alice can use the token as collateral (making sure to use more than UPDATE_MAX_PEG_AMOUNT worth of the token in ETH), borrow ether from MarginSwap, sell the ether for the token on Uniswap, and repeat, leaving coolingOffPeriod blocks between each lend and borrow. (2.2) If the price has risen, Alice can use ether as collateral, borrow the token from MarginSwap (making sure to use more than UPDATE_MAX_PEG_AMOUNT worth of the token in ETH), sell the token for ether on Uniswap, and repeat, leaving coolingOffPeriod blocks between each lend and borrow.
Because the MarginSwap price is now stale, Alice can borrow more than 100% of the actual value of her collateral, since MarginSwap believes the borrowed funds to only be worth 90% or less than their actual current market value.
The various defenses that MarginSwap has employed against undercollateralized loans are all bypassed: (a) The exponential moving price average stored within MarginSwap is not updated, because Alice borrows at least UPDATE_MAX_PEG_AMOUNT worth of the token in ETH, so MarginSwap.PriceAware.getPriceFromAMM skips the price update due to the "outAmount < UPDATE_MAX_PEG_AMOUNT" condition failing. (b) CoolingOffPeriod can be bypassed by Alice splitting her deposits and borrows up by 20 blocks (the current value of CoolingOffPeriod). Since deposits do not trigger a price oracle update, Alice can even deposit ahead of time as the price is nearing 10% off of peg, allowing her to perform the borrow right when 10% is passed. (c) MarginSwap.Lending.registerBorrow check is bypassed. The check is "meta.totalLending >= meta.totalBorrowed", but this is a global check that only ensures that the contract as a whole has sufficient tokens to fund Alice's borrow. Alice simply needs to ensure that she only borrows up to the amount of tokens that the contract currently owns.
Recommended Mitigation Steps
Even if the issue of the price oracle stalling were to be fixed by using a large enough UPDATE_MAX_PEG_AMOUNT, since the moving average updates so slowly (MarginSwap is currently set at moving 8/1000th towards new price every 8 blocks) and only when actions are taken on MarginSwap (which may not be frequent on lesser known tokens or if MarginSwap is not too popular), Alice can still take out undercollateralized loans for a period of time before the price oracle catches up. The real solution here is to use UniswapV2/SushiSwap/UniswapV3's built in TWAP price oracle, especially since MarginSwap is built on top of Uniswap/Sushiswap.