DYAD - ArmedGoose's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L205-L228 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L215

Vulnerability details

Impact

Accounts who have large DYAD mint balance might be immune to liquidations due to requirement stated in the docs - The liquidator burns a quantity of DYAD equal to the target Note’s DYAD minted balance. This may lead to bad debt in the protocol.

Proof of Concept

As per the above, The liquidator burns a quantity of DYAD equal to the target Note’s DYAD minted balance. That means, to liquidate an account, liquidator have to obtain at least the same amount of DYAD as target balance. If a big "whale" accumulates a DYAD mint balance large enough, there won't be a possibility to liquidate unless a group of users will cooperate to gather a significant balance under one account, burn it, and then share rewards, which is a complex activity not included in the protocol logic and might never happen due to lack of mutual trust between groups of users. It is completely possible, that there will be one user/account who has the most DYAD minted, and that user won't be liquidated unless few others cooperate against.

Tools Used

Manual approach

Recommended Mitigation Steps

Allow partial liquidations, based on arbitrary amount of DYAD liquidated, instead of requiring to match target DYAD balance to perform a liquidation.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L127 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L143

Vulnerability details

Impact

A malicious user might be able to grief withdrawals of someone else by depositing 1 wei to a vault[id] associated with victim, triggering save of idToBlockOfLastDeposit[id]. Note, that the vault address does not have "Licensed" check so in fact, attacker doesn't really need to deposit to the victim.

Proof of Concept

In deposit, the idToBlockOfLastDeposit[id] can be triggered by anyone. On Withdraw, which is also part of a redeem logic, it's checked if deposit to corresponding vault ID happened already. The corresponding logic is attached in Links to affected code - with exact lines.

This requires a deposit per every block to be griefed, thus the DoS is not permanent.

However, there is a caveat - since deposit does not check if user is really depositing to added vault but use can specify any address, also a fake vault, which may export respective functions, and just allow calling deposit(id, amount). They key is, idToBlockOfLastDeposit is toggled for the block.

  function deposit(
    uint    id,
    address vault, //@audit can be arbitrary
    uint    amount
  ) 
    external 
      isValidDNft(id) //@audit check if ID exists
  {
    idToBlockOfLastDeposit[id] = block.number;//@audit save deposit
    Vault _vault = Vault(vault); //@audit can be arbitrary
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount); 
    _vault.deposit(id, amount); 
  }

Tools Used

Manual approach

Recommended Mitigation Steps

As with QA finding 1, where similar approach is recommended to protect remove vault functionality - the owner should have possibility to disallow deposits from other users, either by pausing deposits / limiting them to owner only, so external influence on user's state wont work.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L205-L228

Vulnerability details

Impact

Users who liquidate undercollateralized accounts make insignificant profits or even suffer loss, while liquidated accounts do not suffer significant losses. This may lead to bad debt in the protocol as hardly anyone will be willing to liquidate other users.

Proof of Concept

It might be that DYAD simply is wrongly burned off msg.sender instead of owner(to) in this line, however documentation confirms the code so it seems to be intended.

Therefore, below explaining what happens in current state of the protocol. Assume following scenario.

• Bob deposits 2500 USD worth of WETH as a collateral and mints 1500 DYAD $. CR = 1.66
• WETH drops in price on a sudden market drop. Now it happens that Bob's collateral is worth 1200USD. CR = 1.0 after cap, so account can be liquidated
• Alice, who has 1500DYAD minted, decides to liquidate Bob's account (ID) to herself. When Alice calls liquidate:
  • Alice is burned 1500$ DYAD
  • liquidationEquityShare = 0; so liquidationAssetShare is 100%
  • For each Bob's vault, collateral is equal to amount in the vault; since its 100% then just all Bob's collateral is moved to Alice's id.
  • Bob still has 1500DYAD minted (1500USD) which he can sell on the market
  • Alice lost 1500 USD DYAD but made 1200 USD from Bob's collateral. So Alice lost $300 on the liquidation + gas costs.

Let's change the scenario, if Bob's collateral dropped only a bit, say to 2100$. then CR = 1.4, so account is liquidatable. Then liquidationEquityShare = 0.4*0.2=0.02, which causes liquidationAssetShare to be around 77%. So Bob's collateral that will be liquidated is circa 2100 * 77% = 1617 Now Alice could do a small profit (=1617-1500-gas)assuming gas cost won't be high.

For collateral drop to 2000, where CR is circa 1.3 => 0.3 * 0.2 = 0.06 so circa 82% * 2000 = 1660.

In the same time, Bob's DYAD is still there. While he won't be able to get remaining collateral, he can just market sell DYAD and will not lose much.

It clearly seems, that when DYAD is burned off liquidator and not liquidated account, there is no significant profit, thus it is doubtful that in current state anyone should be eager to liquidate undercollateralized accounts.

Tools Used

Manual approach

Recommended Mitigation Steps

The burned DYAD should be burned off liquidated account, not off the liquidator's.

Assessed type

Other

Judge has assessed an item in Issue #892 as 2 risk. The relevant finding follows:

[L-1] Griefing is possible on vault removal In VaultManagerV2.sol line 101 Since anyone can deposit into a vault, someone can deny removal of a vault if will be able to frontrun removal transaction with a 1 wei deposit. However, this is rated as low at best, since this doesn’t look like efficient and impactful attack vector.

Remediation: Either allow owner to pause deposits or implement minimum deposit to make attack more expensive.

Judge has assessed an item in Issue #892 as 2 risk. The relevant finding follows:

[L-1] Griefing is possible on vault removal In VaultManagerV2.sol line 101 Since anyone can deposit into a vault, someone can deny removal of a vault if will be able to frontrun removal transaction with a 1 wei deposit. However, this is rated as low at best, since this doesn’t look like efficient and impactful attack vector.

Remediation: Either allow owner to pause deposits or implement minimum deposit to make attack more expensive.