DYAD - sashik_eth's results

Lines of code

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

Vulnerability details

Users could withdraw collateral tokens using the withdraw function:

File: VaultManagerV2.sol
133:   /// @inheritdoc IVaultManager
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);
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(); 
151:     _vault.withdraw(id, to, amount);
152:     if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
153:   }

As we can see in line 143 - this function checks if there were deposits on behalf of the current note in the same block.

At the same time, the deposit function allows anybody to deposit any amount on another's notes behalf and set idToBlockOfLastDeposit to the latest block:

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:   }

All this opens a griefing vector when the attacker deposits dust amount (or even 0 amount since the deposit function fails to check if the vault address is a real vault), resets the idToBlockOfLastDeposit value for desired note, and blocks withdrawing calls for this note in the current block. The attack could be repeated multiple times DOSing any note withdrawals.

Impact

Withdrawing could be DOSed using dust deposits

Proof of Concept

Consider the next scenario:

1. Alice creates a new position by depositing collateral and minting DYAD.
2. Bob sends deposit calls with Alice's note ID for each next block, permanently blocking Alice's withdrawal call.

Recommended Mitigation Steps

Consider disallowing deposits to notes that the user doesn't own.

Assessed type

Access Control

Lines of code

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

Vulnerability details

The protocol includes two vaults that hold Kerosene tokens - UnboundedKerosineVault and BoundedKerosineVault. While withdrawing from the bounded tokens is impossible by design, withdrawing from the unbounded vault should be available for the users.

Two functions could be used for withdrawing assets from the vaults - withdraw and redeemDyad. The last one uses withdraw inside, so we are interested only in the first one:

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);
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(); 
151:     _vault.withdraw(id, to, amount);
152:     if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
153:   }

As we can see in line 148 - the contract executes the call to vault.oracle() function, however, UnboundedKerosineVault lacks implementation of such a function. This means that any call to withdraw and redeemDyad with the UnboundedKerosineVault address as a vault parameter would always revert.

Impact

Users would not be able to withdraw Kerosene from the UnboundedKerosineVault, while they should.

Proof of Concept

Consider the next scenario:

1. Alice receives Kerosene tokens as a drop and deposits them to UnboundedKerosineVault to use them as a part of the collateral for a future position.
2. Alice changed her mind and decided to sell Kerosene on the secondary market, she tried to withdraw her tokens but tx reverted since the contract tried to call the non-existing function oracle() on the UnboundedKerosineVault address.

Recommended Mitigation Steps

Consider skipping the evaluation value inside the withdraw function for the UnboundedKerosineVault vault.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L65

Vulnerability details

UnboundedKerosineVault#assetPrice function does not count TVL from V1 vaults

UnboundedKerosineVault#assetPrice function calculates the price of the Kerosene token as a collateral asset:

File: Vault.kerosine.unbounded.sol
50:   function assetPrice() 
51:     public 
52:     view 
53:     override
54:     returns (uint) {
55:       uint tvl;
56:       address[] memory vaults = kerosineManager.getVaults();
57:       uint numberOfVaults = vaults.length;
58:       for (uint i = 0; i < numberOfVaults; i++) {
59:         Vault vault = Vault(vaults[i]);
60:         tvl += vault.asset().balanceOf(address(vault)) 
61:                 * vault.assetPrice() * 1e18
62:                 / (10**vault.asset().decimals()) 
63:                 / (10**vault.oracle().decimals());
64:       } 
65:       uint numerator   = tvl - dyad.totalSupply(); 
66:       uint denominator = kerosineDenominator.denominator(); 
67:       return numerator * 1e8 / denominator; 
68:   }

As we can see numerator value depends on two values - the totalSupply of DYAD that could be minted by vaultManagerV2 and already minted by vaultManagerV1, and the tvl value, which here is equal to the sum of all collateral value from vaults that added to kerosineManager list. From the deployment script it's clear that only V2 vaults Vault.sol and Vault.wsteth.sol would be added to this list:

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

However, current vaults from V1 already hold some amount of collateral tokens and there is some amount of DYAD token that minted against this collateral. The protocol does not have a way to force the burning of these tokens and returning collateral to initial minters. This means that the numerator value would be incorrectly calculated because of counting TVL only from V2 vaults but DYAD tokens(total supply) from both V1 and V2 versions.

Impact

Kerosene price would be incorrectly calculated, potentially blocking minting against Kerosene token, which is the core functionality of the protocol.

Proof of Concept

Consider the next scenario:

1. Alice creates a position in vaultManager V1 using a $150 collateral value of WETH, minting 100 DYAD tokens. Current total supply of DYAD = 100.
2. Bob creates a position in vaultManager V2 using a $150 collateral value of WETH, minting 100 DYAD tokens. Current total supply of DYAD = 200.
3. Charlie trying to create the position in vaultManager V2 using the $100 collateral value of WETH and 1000 Kerosene tokens, Charlie currently holds 100 percent of Kerosene free supply, this means he should be able to utilize all surplus collateral, however, his tx reverts since assetPrice() encounters only Bob's collateral ($150) while the total supply of DYAD (200).

Should note that users of V1 manager in this way intentionally or accidentally could DOS using Kerosene by opening/increasing positions in V1.

Recommended Mitigation Steps

Consider updating the UnboundedKerosineVault#assetPrice function so it's counting collateral value from V1 vaults in total TVL.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L65

Vulnerability details

The assetPrice() function calculates price of Kerosene in both Kerosene vaults:

File: Vault.kerosine.unbounded.sol
50:   function assetPrice() 
51:     public 
52:     view 
53:     override
54:     returns (uint) {
55:       uint tvl;
56:       address[] memory vaults = kerosineManager.getVaults();
57:       uint numberOfVaults = vaults.length;
58:       for (uint i = 0; i < numberOfVaults; i++) {
59:         Vault vault = Vault(vaults[i]);
60:         tvl += vault.asset().balanceOf(address(vault)) 
61:                 * vault.assetPrice() * 1e18
62:                 / (10**vault.asset().decimals()) 
63:                 / (10**vault.oracle().decimals());
64:       } 
65:       uint numerator   = tvl - dyad.totalSupply();
66:       uint denominator = kerosineDenominator.denominator(); 
67:       return numerator * 1e8 / denominator; 
68:   }

This function is called during the execution of liquidation in the case the user previously added any Kerosene vault to his note independently if there is any Kerosene deposit for this note or not.

As we can see in line 65 - if the tvl value becomes less than the dyad.totalSupply() execution of the function would be reverted due to overflow. All this could lead to a scenario when after a rapid drop in the prices of collateral execution of the liquidations would be DOSed, resulting in locked collateral inside of the protocol.

Impact

Collateral tokens would be locked if TVL would drop less than the totalSupply of DYAD, liquidations would revert in this case.

Proof of Concept

Consider the next scenario:

1. Alice deposits $150 collateral of WETH, minting 100 DYAD.
2. Bob deposits $100 collateral of WETH and some amount of Kerosene, minting 100 DYAD.
3. Currently there is $250 of non-kerosene collateral and 200 DYAD in protocol.
4. The price of WETH drops by 25% and now non-kerosene collateral value inside the protocol is equal to ~$187.
5. Charlie trying to liquidate both positions but his TX reverts due to overflow inside assetPrice on Kerosine vaults (both depositors added Kerosine vaults to their notes). Since positions were "unhealthy", depositors also could not withdraw collateral without additional deposits.

Recommended Mitigation Steps

Consider returning 0 price for the Kerosene token in case TVL is less than the totalSupply of DYAD. This would allow the execution of liquidations even in these conditions, preventing locking collateral assets inside the protocol.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L65

Vulnerability details

The assetPrice() function calculates the Kerosene price:

File: Vault.kerosine.unbounded.sol
50:   function assetPrice() 
51:     public 
52:     view 
53:     override
54:     returns (uint) {
55:       uint tvl;
56:       address[] memory vaults = kerosineManager.getVaults();
57:       uint numberOfVaults = vaults.length;
58:       for (uint i = 0; i < numberOfVaults; i++) {
59:         Vault vault = Vault(vaults[i]);
60:         tvl += vault.asset().balanceOf(address(vault)) 
61:                 * vault.assetPrice() * 1e18
62:                 / (10**vault.asset().decimals()) 
63:                 / (10**vault.oracle().decimals());
64:       }
65:       uint numerator   = tvl - dyad.totalSupply();
66:       uint denominator = kerosineDenominator.denominator(); 
67:       return numerator * 1e8 / denominator;
68:   }

It uses the tvl value to evaluate how much surplus collateral exists in the protocol (L65). The problem is that not all values deposited to the vaults are actually locked. Users can deposit collateral and withdraw it later freely in case it's not back a minimal percent (150%) for minted DYAD. This means that the price of Kerosene would be inflated by this collateral, potentially opening doors for risk-free trades or trades with hedged risk. In bad cases, this would affect holders of DYAD tokens as shown in the POC lower.

Impact

TVL wrongly includes collateral that is not used for minting, this potentially allows users to create risk-free long positions for collateral tokens.

Proof of Concept

Consider the next scenario (for simplicity let's assume that Alice is the only depositor, she holds all "free" Kerosene, and price of WETH and WSTETH could move independently for some percent):

1. Alice mints two notes with IDs 1 and 2. Her initial balance is $3k USDC.
2. Alice swap $1k to WETH and deposit it per note with ID 1.
3. Alice swap $2k to WSTETH and deposit it per note with ID 2. Also, Alice deposits all her Kerosene tokens per this note.
4. Alice mints $2k of DYAD per note with ID 2. Its collar ratio is currently 1.5 due to surplus from step 2 and Kerosene usage.
5. Alice sold $2k of DYAD for $2k of USDC to Bob on the secondary market. 6a. If the price of WSTETH goes up Alice simply does all steps backward, profiting from a successful long position. 6b. If the price of WSTETH goes down Alice withdraws WETH for a note with ID 1 sells it per $1k USDC back and resulting in no losses. Bob continues to hold DYAD tokens that currently have collateral of less than $1 per token, he could liquidate Alice's position and receive all Kerosene tokens, but it would be now useless since no surplus collateral is left in the protocol.

As a result, Alice received the possibility of a risk-free position using Bob's liquidity.

Recommended Mitigation Steps

Consider updating protocol logic, so it separately encounters collateral that is used as a minimum (150%) collateral for minted positions as TVL.

Assessed type

Other

Lines of code

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

Vulnerability details

The liquidate function allows the liquidator to receive a portion of the liquidated position, this is done by iterating through positions vaults and moving the appropriate part of its assets to the liquidator's note (L222):

File: VaultManagerV2.sol
205:   function liquidate(
206:     uint id,
207:     uint to
208:   ) 
209:     external 
210:       isValidDNft(id)
211:       isValidDNft(to)
212:     {
213:       uint cr = collatRatio(id);
214:       if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
215:       dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));
216: 
217:       uint cappedCr               = cr < 1e18 ? 1e18 : cr;
218:       uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); 
219:       uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);
220: 
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:   }

While it's working correctly for the V1 version, in V2 each note could also have assets in keroseneVaults. A portion of Kerosene in collateral value could go up to 50% of the needed minimum of 150%. As Kerosene vaults take part in the collateral ratio of the position - it's also should be moved proportionally to the liquidator's note But as we can see liquidate functions lack to iterate through Kerosene vaults and move part of liquidated position assets to the liquidator's note, which means that liquidators would miss part of its assets.

Impact

Liquidators would not receive part of their expected value or would have a lack of economic motivation to liquidate positions.

Proof of Concept

Consider the next scenario:

1. Alice mints 100 DYAD using $100 (0.1 WETH) collateral and some amount of Kerosene token, her position collateral ratio is 1.5.
2. The price of WETH goes 10% down, now Alice's position ratio is less than 1.5, meaning that it's liquidatable.
3. Bob repays 100 DYAD (costing $100) of Alice's debt, liquidating her position. But he receives only some part of WETH collateral, which currently even at most (0.1) is equal to only $90. Bob is missing any Kerosene tokens from Alice's position.

Recommended Mitigation Steps

Consider updating the liquidate function so it iterates through the Kerosene vaults of the liquidated notes in the way it is already done with non-Kerosene vaults.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Users can add and later remove vault addresses from their notes. Removing could be done using remove and removeKerosene functions:

File: VaultManagerV2.sol
094:   function remove(
095:       uint    id,
096:       address vault
097:   ) 
098:     external
099:       isDNftOwner(id)
100:   {
101:     if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets(); 
102:     if (!vaults[id].remove(vault))     revert VaultNotAdded();
103:     emit Removed(id, vault);
104:   }
105: 
106:   function removeKerosene(
107:       uint    id,
108:       address vault
109:   ) 
110:     external
111:       isDNftOwner(id)
112:   {
113:     if (Vault(vault).id2asset(id) > 0)     revert VaultHasAssets(); 
114:     if (!vaultsKerosene[id].remove(vault)) revert VaultNotAdded();
115:     emit Removed(id, vault);
116:   }

As we can see each function during removing checks if the note position balance in the desired vault is equal to 0 and reverts if opposite (L101 and L114). Since anyone could deposit an arbitrary amount of collateral on behalf of other's notes, this opens a griefing vector when attacker DOS removes vault addresses by sending dust amount deposits to other users' notes.

Impact

Anyone could DOS execution of remove and removeKerosene functions

Proof of Concept

Consider the next scenario:

1. Alice adds the Vault address to her note with id 1.
2. Alice decided to remove the previously added address from her note, sending TX with a call to the remove function.
3. Bob frontrun Alice tx with dust deposit tx to her note with id 1, now id2asset() function returns a value greater than 0, blocking the execution of Alice tx.

Recommended Mitigation Steps

Consider disallowing deposits to notes that the user doesn't own.

Assessed type

Access Control

Lines of code

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

Vulnerability details

The add function allows users to add vaults to a list of non-kerosene collateral vaults per their notes, it checks if an added vault is licensed in vaultLicenser (L75):

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:   }

If we check the deployment script, we can see that both Kerosene and non-kerosene types of vaults would be added to the licenser:

File: Deploy.V2.s.sol
93:     vaultLicenser.add(address(ethVault));
94:     vaultLicenser.add(address(wstEth));
95:     vaultLicenser.add(address(unboundedKerosineVault));

All this means that users would be able to add an unboundedKerosineVault vault address to their vaults[id] list. The problem is that the getNonKeroseneValue function iterates through this list and calculates the sum of non-kerosene collateral for the given note id:

File: VaultManagerV2.sol
250:   function getNonKeroseneValue(
251:     uint id
252:   ) 
253:     public 
254:     view
255:     returns (uint) {
256:       uint totalUsdValue;
257:       uint numberOfVaults = vaults[id].length(); 
258:       for (uint i = 0; i < numberOfVaults; i++) {
259:         Vault vault = Vault(vaults[id].at(i));
260:         uint usdValue;
261:         if (vaultLicenser.isLicensed(address(vault))) {
262:           usdValue = vault.getUsdValue(id);        
263:         }
264:         totalUsdValue += usdValue;
265:       }
266:       return totalUsdValue;
267:   }

This allows users to inflate their non-kerosene collateral amount and total collateral ratio by adding the Kerosene vault to the non-kerosene vault list.

Impact

Users could inflate their collateral ratio by adding Kerosene vaults as non-kerosene

Proof of Concept

Consider the next scenario:

1. Alice adds the unboundedKerosineVault address using the add function.
2. Alice deposits $150 in Kerosene tokens, minting 100 DYAD tokens against it.

Here one of the main protocols invariant (1 DYAD should be minted with at least $1 of exogenous collateral) is broken.

There are also scenarios with this bug - e.g. when the same Kerosene tokens are counted two times (as non-kerosene and kerosene vault values) or when liquidation of such position is blocked since the liquidator part would be tried to move twice, etc. But the main idea is clear - that the Kerosene vault should not ever be added to the non-kerosene vaults[id] list.

Recommended Mitigation Steps

Consider updating the add function in the way it checks if the added vault is not a Kerosene vault.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Users should be able to add Kerosene vaults to their notes using addKerosene function:

File: VaultManagerV2.sol
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:   }

At line 88 this function includes checking if the given vault is licensed in the keroseneManager contract. However, the keroseneManager contract actually contains addresses of the non-kerosene vault. The purpose of its contract is to provide list of non-kerosene vaults for assetPrice(). We could ensure this by checking the deployment script:

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

This means that the Kerosene vaults would not be possible to add per user's notes, breaking using the Kerosene token as collateral, which is one of the core functionalities of the protocol.

Also, Kerosene vaults were wrongly checked using the keroseneManager inside the getKeroseneValue function (L280):

File: VaultManagerV2.sol
269:   function getKeroseneValue(
270:     uint id
271:   ) 
272:     public 
273:     view
274:     returns (uint) {
275:       uint totalUsdValue;
276:       uint numberOfVaults = vaultsKerosene[id].length(); 
277:       for (uint i = 0; i < numberOfVaults; i++) {
278:         Vault vault = Vault(vaultsKerosene[id].at(i));
279:         uint usdValue;
280:         if (keroseneManager.isLicensed(address(vault))) {
281:           usdValue = vault.getUsdValue(id);        
282:         }
283:         totalUsdValue += usdValue;
284:       }
285:       return totalUsdValue;
286:   }
287: 

Impact

Users would not be able to add Kerosene vaults to their notes.

Recommended Mitigation Steps

Consider creating a separate licenser for Kerosene vaults and check vault addresses using it.

Assessed type

Access Control