Fujicracy / fuji-v2

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

Fix DoS Borrowing Vault #377

Closed pedrovalido closed 1 year ago

pedrovalido commented 1 year ago

[H-1] Someone can execute a Denial of Service on Fuji’s Borrowing vault.

Description

Multiple lending providers offer the option to repay a loan on someone else's behalf. For example, Aave V2 allows this feature.

contracts/protocol/lendingpool/LendingPool.sol#L236

function repay(
    address asset,
    uint256 amount,
    uint256 rateMode,
    address onBehalfOf
  ) external override whenNotPaused returns (uint256) {
    DataTypes.ReserveData storage reserve = _reserves[asset];

It's possible for someone to leverage this feature and pay off vault debts. The Fuji vault currently does not handle this case, halting both borrowing and payback, locking collateral for borrowers.

The reason is convertDebtToShares would revert due to division by 0

function _convertDebtToShares(
    uint256 debt,
    Math.Rounding rounding
  )
    internal
    view
    returns (uint256 shares)
  {
    uint256 supply = debtSharesSupply;
    return (debt == 0 || supply == 0) ? debt : debt.mulDiv(supply, totalDebt(), rounding);
  }

While paying a vault's debt is theoretically possible, one may question why someone would do so.

We acknowledge that it is less probable for individuals to pay off significant debts, but in the early stages of the vault, when the debt is minimal or even worth 1 wei, one can pay it on the vault's behalf and execute a denial of service on further debt actions on the vault for a lifetime.

The only option for Fuji in this scenario would be to redeploy the vault and hope that attackers do not grief it again.

Remediation to consider

We couldn't come up with an ideal isolated solution to this problem. Please let us know if you have any ideas.

There are some possible ways, like considering shares to mint equal to debt only in _convertDebtToShares, like it's done if the supply of shares is zero. However, this approach would distribute the new debt position over all previous borrowers, which is not ideal and could create additional complexity and attack vectors.

Instead, as mitigation after deployment, consider borrowing a significant amount yourself to reduce the likelihood of someone repaying the vault's debt on their own. Even in the extreme case where someone pays off the entire vault's debt, users should still be able to withdraw their funds. Consider adding a test for same.

pedrovalido commented 1 year ago

check how morpho does it

pedrovalido commented 1 year ago

check deployment scripts. check vanila router PR

pedrovalido commented 1 year ago

chosen option: in the beginning there are not debtShares so no problem.

If someone repays the the vaults debt, we call an admin function with requirements (debt = 0 and debtShares > 0) that borrows input amount -> check input amount ???