DYAD - 3docSec's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L93-L94 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L67-L91 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L241-L248

Vulnerability details

VaultManagerV2 allows adding vaults as regular vaults or as vaultsKerosene for a given NFT. The contract does however not prevent adding a single vault to both collection, causing its assets to be double-counted in the collateral calculations, because when adding a vault, the only check that is made is that it's eligible and not added already to the list:

File: VaultManagerV2.sol
67:   function add(
68:       uint    id,
69:       address vault
70:   ) 
71:     external
72:       isDNftOwner(id)
73:   {
74:     if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
75:     if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
76:     if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
77:     emit Added(id, vault);
78:   }
79: 
80:   function addKerosene(
81:       uint    id,
82:       address vault
83:   ) 
84:     external
85:       isDNftOwner(id)
86:   {
87:     if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
88:     if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
89:     if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
90:     emit Added(id, vault);
91:   }
92: 

It is therefore possible that a vault that is at the same time in the allowlist of keroseneManager and vaultLicenser is added to both collections of the same NFT id.

This is an extremely likely scenario because if we look at the deployment script, we find two examples of vaults in both allowlists:

File: Deploy.V2.s.sol
64:     kerosineManager.add(address(ethVault));
65:     kerosineManager.add(address(wstEth));
---
93:     vaultLicenser.add(address(ethVault));
94:     vaultLicenser.add(address(wstEth));

When this happens, the VaultManagerV2 will double-count the value locked in the vault, because the calculations on vaults and vaultsKerosene happen separately:

File: VaultManagerV2.sol
230:   function collatRatio(
231:     uint id
232:   )
233:     public 
234:     view
235:     returns (uint) {
236:       uint _dyad = dyad.mintedDyad(address(this), id);
237:       if (_dyad == 0) return type(uint).max;
238:       return getTotalUsdValue(id).divWadDown(_dyad);
239:   }
240: 
241:   function getTotalUsdValue(
242:     uint id
243:   ) 
244:     public 
245:     view
246:     returns (uint) {
247:       return getNonKeroseneValue(id) + getKeroseneValue(id);
248:   }

Impact

Malicious actors can add a vault to both vaults and vaultsKerosene to open undercollateralized borrower positions.

Proof of Concept

• call add and addKerosene for the same vault
• deposit $1k worth of collateral for that vault
• borrow $1k worth of stablecoin (while borrowing $1.3k would be theoretically possible, the NotEnoughExoCollat extra check would prevent that)
• the position can be liquidated only when heavily insolvent

Tools Used

Code review, Foundry

Recommended Mitigation Steps

• On VaultManagerV2.add, add a cross-check on vaultsKerosene[id] to ensure the vault to be added is not there already
• On VaultManagerV2.addKerosene, add a cross-check on vaults[id] to ensure the vault to be added is not there already

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L119-L131 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L143-L143

Vulnerability details

The VaultManagerV2 withdraw function prevents withdraws from positions that have received a deposit in the same block:

File: VaultManagerV2.sol
134:   function withdraw(
135:     uint    id,
136:     address vault,
137:     uint    amount,
138:     address to
139:   ) 
140:     public
141:       isDNftOwner(id)
142:   {
143:     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
144:     uint dyadMinted = dyad.mintedDyad(address(this), id);

However, there is almost no check on the deposit function which creates the condition for the withdraw to revert:

File: VaultManagerV2.sol
119:   function deposit(
120:     uint    id,
121:     address vault,
122:     uint    amount
123:   ) 
124:     external 
125:       isValidDNft(id)
126:   {
127:     idToBlockOfLastDeposit[id] = block.number;
128:     Vault _vault = Vault(vault);
129:     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
130:     _vault.deposit(id, amount);
131:   }

In particular, only id is validated, while:

• vault can be anything, including a malicious contract
• vault.asset() can therefore return anything, including a mock ERC-20
• amount can also be 0, allowing the exploit to work even with a legitimate vault
• msg.sender is not validated to be related to the NFT id

Impact

Anybody can DoS withdraw operations on any id by simply front-running them with a deposit call for that same id, with amount = 0, or a dummy vault. This can also be an effective way to prevent liquidations in case the liquidator withdraws transactionally.

Proof of Concept

• Alice submits a withdraw transaction to the mempool
• Bob observes it, and front-runs Alice's transaction with a matching deposit with amount = 0
• Alice's withdraw fails

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider removing the DepositedInSameBlock check, or changing the modifier of deposit from isValidDNft to isDNftOwner.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L146-L150

Vulnerability details

VaultManagerV2 has a protection in place to make sure users don't withdraw non-kerosene collateral backing their debt positions:

File: VaultManagerV2.sol
134:   function withdraw(
---
144:     uint dyadMinted = dyad.mintedDyad(address(this), id);
145:     Vault _vault = Vault(vault);
146:     uint value = amount * _vault.assetPrice() 
147:                   * 1e18 
148:                   / 10**_vault.oracle().decimals() 
149:                   / 10**_vault.asset().decimals();
150:     if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

This implementation can become problematic when one attempts to withdraw an amount from a vault that is not registered in vaults[id] (as the code assumes) but rather in vaultsKerosene[id]. It is therefore possible that the value removed is higher than the total value locked in the vaults[id] vaults. This situation will cause

Impact

Withdraws from kerosene vaults will fail when the amount withdrawn is higher than the collateral locked in the non-kerosene vaults.

Proof of Concept

• Have an id that has only one kerosene vault attached
• Deposit any amount
• Withdraw any amount, this will revert

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider enforcing the L150 requirement only in case vault is counted in the getNonKeroseneValue calculation:

    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();
-   if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
+   if (vaults[id].contains(vault) && vaultLicenser.isLicensed(vault) &&
+       getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/VaultManagerV2.sol#L221-L226

Vulnerability details

The key concept of this finding is the inconsistency in VaultManagerV2 between what backs the value of the lent dyad.mintedDyad(address(this), id) tokens:

• the collatRatio function that counts assets in both vaultsKerosene[id] and vaults[id]
• the liquidate function only awards the liquidator the tokens in vaults[id]
• its calculation, however, bases its liquidationAssetShare calculation on the collatRatio as if also vaultsKerosene[id] tokens were to be distributed

File: VaultManagerV2.sol
230:   function collatRatio(
231:     uint id
232:   )
233:     public 
234:     view
235:     returns (uint) {
236:       uint _dyad = dyad.mintedDyad(address(this), id);
237:       if (_dyad == 0) return type(uint).max;
238: @>    return getTotalUsdValue(id).divWadDown(_dyad);
239:   }
240: 
241:   function getTotalUsdValue(
242:     uint id
243:   ) 
244:     public 
245:     view
246:     returns (uint) {
247: @>    return getNonKeroseneValue(id) + getKeroseneValue(id);
248:   }

File: VaultManagerV2.sol
205:   function liquidate(
206:     uint id,
207:     uint to
208:   ) 
209:     external 
210:       isValidDNft(id)
211:       isValidDNft(to)
212:     {
---
221: @>    uint numberOfVaults = vaults[id].length();
222:       for (uint i = 0; i < numberOfVaults; i++) {
223:           Vault vault      = Vault(vaults[id].at(i));
224:           uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
225:           vault.move(id, to, collateral);
226:       }
227:       emit Liquidate(id, msg.sender, to);
228:   }

This means that if at any point in time, the value of tokens in vaultsKerosene[id] becomes significantly higher than that of vaults[id], the lending position id can be significantly insolvent at the time it enters liquidation territory, and who attempts to liquidate it, will inevitably incur a loss.

Impact

A borrower can open positions that are impossible to liquidate for a profit in general, and even more effectively when picking sizable (vaults) and non-sizable (vaultsKerosene) assets that are correlated with one guaranteed to appreciate in value over the other.

Proof of Concept

• deposit $1k in wETH (vault)
• deposit $1k in wStETH (kerosene)
• borrow $1k (ratio is 2, the position is healthy & passes both CrTooLow and NotEnoughExoCollat checks)
• both collaterals halve in price (ratio becomes 1, the position is unhealthy)
• a liquidator is awarded the maximum liquidationAssetShare (100%), of wETH only
• the liquidator is at loss: $1k debt was repaid in exchange for $500 in wETH

The liquidator loss can be further exacerbated if, in addition to the price drop of both tokens, wStEth increased in value over wETH:

• deposit $1k in wETH (vault)
• deposit $1k in wStETH (kerosene)
• borrow $1k (ratio is 2)
• wStEth appreciates 50% over wETH -> (ratio becomes 2.5)
• both collaterals halve in price (ratio becomes 1.25)
• a liquidator is awarded a liquidationAssetShare of 84%, of wETH only
• the liquidator is again at loss: $1k debt was repaid in exchange for $420 in wETH

Tools Used

Code review, Foundry

Recommended Mitigation Steps

There are different ways to solve this issue:

• one is to make the kerosene vaults sizable at liquidation time (in case the team did not intentionally leave these out of liquidation)
• another possibility is to:
  • add the solvency check getNonKeroseneValue(id) < dyadMinted (that is already enforced in several places) as an extra criteria for allowing liquidations
  • calculate liquidationAssetShare in a way that takes into account only the seizable assets

Assessed type

Math