search

code-423n4 / 2021-05-yield-findings

0 stars 0 forks source link

Users can avoid paying borrowing interest after the fyToken matures #71

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

shw

Vulnerability details

Impact

According to the protocol design, users have to pay borrowing interest when repaying the debt with underlying tokens after maturity. However, a user can give his vault to Witch and then buy all his collateral using underlying tokens to avoid paying the interest. Besides, this bug could make users less incentivized to repay the debt before maturity and hold the underlying tokens until liquidation.

Proof of Concept

  1. A user creates a new vault and opens a borrowing position as usual.
  2. The maturity date passed. If the user wants to close the position using underlying tokens, he has to pay a borrowing interest (line 350 in Ladle), which is his debt multiplied by the rate accrual (line 373).
  3. Now, the user wants to avoid paying the borrowing interest. He gives his vault to Witch by calling the function batch of Ladle with the operation GIVE.
  4. He then calls the function buy of Witch with the corresponding vaultId to buy all his collateral using underlying tokens.

In the last step, the elapsed time (line 61) is equal to the current timestamp since the vault is never grabbed by Witch before, and thus the auction time of the vault, cauldron.auctions(vaultId), is 0 (the default mapping value). Therefore, the collateral is sold at a price of balances_.art/balances_.ink (line 74). The user can buy balances_.ink amount of collateral using balances_.art but not paying for borrowing fees.

Referenced code: Ladle.sol#L350 Ladle.sol#L368-L377 Ladle.sol#L267-L272 Cauldron.sol#L234-L252 Witch.sol#L61 Witch.sol#L74

Recommended Mitigation Steps

Do not allow users to give vaults to Witch. To be more careful, require vaultOwners[vaultId] and cauldron.auctions(vaultId) to be non-zero at the beginning of function buy.

alcueca commented 3 years ago

That's a good catch. The mitigation steps are right to avoid this being exploited by malicious users, but it would be better to fix the underlying issue.

The problem is that the Witch always applies a 1:1 exchange rate between underlying and fyToken, and that is not true after maturity. As long as this is not fixed, the protocol will lose money on after maturity liquidations.

alcueca commented 3 years ago

More specifically, _debtInBase should be a Cauldron public function, and return (debtInFYToken, debtInBase). The Ladle would save a bit in deployment gas, the Witch would use it to find out the underlying / fyToken exchange rate.

alcueca commented 3 years ago

However, since grab wouldn't be called on the Witch, the vault owner wouldn't be registered. With the liquidation being a Dutch auction, the vault owner would only get a portion of his collateral back after paying all the debt. The vault with the remaining collateral would be given to address(0).

There is a small protocol loss from miscalculated vault debt on vaults liquidated after maturity, but no user funds would be at risk.

I would propose a severity of 2, given there are monetary losses, however slight, to the protocol. The attack described can't happen, but it revealed a real issue.