search

Fujicracy / fuji-v2

Cross-chain money market aggregator
https://fuji-v2-frontend.vercel.app
15 stars 10 forks source link

Incorrect gainedShared calculation when call function BorrowingVault.liquidate #311

Closed trungore closed 1 year ago

trungore commented 1 year ago

Title

Incorrect gainedShared calculation when call function BorrowingVault.liquidate

Affected smart contract

https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L604-L606

Description

Function FujiOracle._getUSDPrice() requests the price from chainlink oracle with a given asset. The chainlink then returns the price of 1 "real" token asset (not 1 wei) in usd which means with 10^asset_decimal asset how much usd you can get. From that definition, the function FujiOracle.getPriceOf(address currencyAsset, address commodityAsset, decimals) returns the value: 

(10^commodityDecimal * commodityPrice) / (10^currencyDecimal * currencyPrice) * 10^decimal

In which:

This information is decent to talk about the issue with function BorrowingVault.liquidate(). The issue came from the calculation of variable gainedShares which I reckon that the developer mistakenly consider the decimals() of the asset token is 18. 

/// url = https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L604-L606
uint256 price = oracle.getPriceOf(debtAsset(), asset(), _debtDecimals);
uint256 discountedPrice = Math.mulDiv(price, LIQUIDATION_PENALTY, 1e18);
gainedShares = convertToShares(Math.mulDiv(debt, liquidationFactor, discountedPrice));

I write down these 3 lines on a piece of paper for more detail about the formula.

The problem is about the variable "x" in the image. "x" is the corresponding asset which will be repaid when liquidation happens. As we can see on the last red line of the paper, the correspondAsset * (liqFactor / 10^18) / (penalty / 10^18) is the correct value we want to assign to x. Unfortunately there is a weird fraction 10^18 / 10^decimal appearing in the result which made the whole calculation incorrect.

For example, assume that:

We have: 

gainedShares = convertToShares(x), which: 
    x = Math.mulDiv(debt, liquidationFactor, discountedPrice)
<=> x(USDC) = debts(WETH) * 1e18 (liqFactor) / 6e14 (discountPrice)

given debts(WETH) = 1e18
==> x(USDC) = 1e18 * 1e18 / 6e14 ~= 1666e18 
(too high, expect 1666e6 because the decimal of USDC in 6)
<=> gainedShares = convertToShares(gainedAssets) = convertToShares(1666e18) 
(can't burn min(gainedShares, existingShares) since the owner can still have some debt remain (liquidationFactor < 10^18))

Attack scenario

if the decimal is 18, there won't be any problem with the liquidation. But when the decimal < 18, it will make the calculated gainedShares bigger than expected. It can make the tx revert since the amount of burning share can exceed the maxRedeem(owner) (the burning share in this case will be equal to the share balance of owner but owner can still own some debt share after the liquidation -- liquidationFactor < 10^18). ==> Lead to DOS when calling liquidation ==> bad debt See the PR: https://github.com/Fujicracy/fuji-v2/pull/312

Recommendation

Modify the calculation as follows: 

uint256 price = oracle.getPriceOf(debtAsset(), asset(), _debtDecimals);
uint256 discountedPrice = Math.mulDiv(price, LIQUIDATION_PENALTY, 1e18);

uint256 gainedAsset = Math.mulDiv(debt, liquidationFactor, discountedPrice);
/// change here 
gainedAsset = Math.mulDiv(gainedAsset, 10**decimals(), 10**18);

gainedShares = convertToShares(gainedAsset);

Note: I saw that you guy just used 18 decimals to describe the tests. I recommend adding more tests with other decimals for diversification. It can help to cover more edge cases though.

0xdcota commented 1 year ago

This issue was resolved in PR #384.