Fujicracy / fuji-v2

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

M-2 Vault rounding #449

Closed 0xdcota closed 1 year ago

0xdcota commented 1 year ago

[M-2] Incorrect Rounding for Shares/Assets/Debt Calculation

Description

All rounding should be done considering the best result for the vault and the worst result for users so that someone cannot take advantage of the vault with consecutive actions without losing any value. However, Fuji vaults perform incorrect rounding in multiple instances (all instances listed [here](https://www.notion.so/46dde2fad9c7487b9888b3838b02153c)), allowing users to extract more than their contribution.

For example, consider Fuji's previewWithdraw function, which rounds down instead of rounding up:

Shares (Alice) = 50
TotalSupplyShares = 100
TotalAssets = 200

Alice calls withdraw for one asset,

previewWithdraw (round down): the number of shares to be burned is (1 * 100) / 200 = 0.

Hence, Alice can theoretically keep withdrawing one worth of assets without burning any shares. Although the associated gas costs won't make this trade feasible, it breaks the invariant. Hence we classified this issue as a medium rather than high.

Remediation to consider

Consider reversing rounding in the following cases:

  1. previewWithdraw: Round up. You want to burn the maximum number of shares for given assets.
  2. previewMint: Round up. You want to pull the maximum number of assets for given shares.
  3. borrow:

[fuji-v2/BorrowingVault.sol at main · Fujicracy/fuji-v2](https://github.com/Fujicracy/fuji-v2/blob/main/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L235)

uint256 shares = convertDebtToShares(debt);

Round up. You don't want users to add debt to the vault without getting shares for it.

  1. _computeMaxBorrow :

[fuji-v2/BorrowingVault.sol at 6231dd1161602cc4b081fd2bab621fa6a3cb9394 · Fujicracy/fuji-v2](https://github.com/Fujicracy/fuji-v2/blob/6231dd1161602cc4b081fd2bab621fa6a3cb9394/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L351)

uint256 debt = convertToDebt(debtShares);

Round up. You want the user to borrow the least possible.

  1. _computeFreeAssets:

[fuji-v2/BorrowingVault.sol at main · Fujicracy/fuji-v2](https://github.com/Fujicracy/fuji-v2/blob/main/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L365)

uint256 debt = convertToDebt(debtShares);

Round up. You want the user to be responsible for the maximum debt.

  1. getHealthFactor :

[fuji-v2/BorrowingVault.sol at main · Fujicracy/fuji-v2](https://github.com/Fujicracy/fuji-v2/blob/main/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L557)

uint256 debt = convertToDebt(debtShares)

Round up. You want to factor in the maximum possible debt.

  1. liquidate :

[fuji-v2/BorrowingVault.sol at main · Fujicracy/fuji-v2](https://github.com/Fujicracy/fuji-v2/blob/main/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L604)

uint256 debt = convertToDebt(_debtShares[owner]);

Round up. You want the liquidator to cover the maximum possible debt for given shares.