DYAD - MrPotatoMagic's results

Lines of code

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

Vulnerability details

Impact

The liquidate() function allows liquidators to burn DYAD on behalf of an DNft id and receive collateral in return.

The issue is that the current functionality only allows burning of the whole DYAD amount minted by the DNft id. This means that partial liquidations cannot be performed and prevents liquidators from liquidating DYAD minted by whales that hold huge positions in the system. Since the liquidations cannot be performed unless the liquidator can match upto the collateral deposited and DYAD minted by the whale, the system will be undercollaterized causing bad debt to accrue.

The effect of this issue will increase as more such positions exist in the system that cannot be liquidated by the liquidators.

Proof of Concept

In the liquidate() function below, we can see on Line 235 that when the burn() function is called on the DYAD token contract, it burns the whole minted DYAD instead of allowing the liquidator to supply a specific amount they can burn to improve the collateral ratio of the id and the overall health of the system.

But since this is not allowed, liquidators trying to liquidate whales, who have minted a huge amount of DYAD, would fail due to the position being extremely big and the inability of partially liquidate.

File: VaultManagerV2.sol
225:   function liquidate(
226:     uint id,
227:     uint to
228:   ) 
229:     external 
230:       isValidDNft(id)
231:       isValidDNft(to)
232:     {
233:       uint cr = collatRatio(id);
234:       if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
235:       dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));
236: 
237:       
238:       uint cappedCr               = cr < 1e18 ? 1e18 : cr;
239:       uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
240:       uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);
241: 
242:       uint numberOfVaults = vaults[id].length();
243:       for (uint i = 0; i < numberOfVaults; i++) {
244:           Vault vault      = Vault(vaults[id].at(i));
245:           uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
246:           
247:           vault.move(id, to, collateral);
248:       }
249:       emit Liquidate(id, msg.sender, to);
250:   }

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a mechanism to allow liquidators to partially liquidate positions. This would also require refactoring the collateral paid out to them based on the amount they cover.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

Root cause: Function deposit() uses modifier isValidDNft() instead of isDNftOwner(), which allows anyone to call deposit() on behalf of any DNft id.

Impacts:

1. Attacker can make user devoid of withdrawing or redeeming collateral by making 0 value deposit() calls. Other than denying users from temporarily withdrawing their collateral, this is also an issue since it could force users into liquidations when they try to take preventative measures on their collateral ratio (especially through redeemDyad()) in high collateral price volatility situations. If successful, the attacker could then perform the liquidation to profit from the situation.
2. Attacker can make user devoid of removing vault due to id2asset > 0 by depositing 1 wei of collateral.

Proof of Concept

Let's understand the first impact:

1. User calls redeemDyad() (or withdraw() to directly withdraw collateral) to burn DYAD and withdraw their collateral asset.

2. Attacker frontruns the call with a 0 value deposit() call. This would set the idToBlockOfLastDeposit[id] to the current block.number. The call does not revert since 0 value transfers are allowed.

File: VaultManagerV2.sol
127:   function deposit(
128:     uint    id,
129:     address vault,
130:     uint    amount
131:   ) 
132:     external 
133:       isValidDNft(id)
134:   {
135:     idToBlockOfLastDeposit[id] = block.number;
136:     Vault _vault = Vault(vault);
137:     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
138:     _vault.deposit(id, amount);
139:   }

3. When the user's redeemDyad() call goes through, it internally calls the withdraw() function, which would cause a revert due to the check on Line 152. The check evaluates to true and reverts since the attacker changes the last deposit block number to the current block through the 0 value deposit() call.

File: VaultManagerV2.sol
143:   function withdraw(
144:     uint    id,
145:     address vault,
146:     uint    amount,
147:     address to
148:   ) 
149:     public
150:       isDNftOwner(id)
151:   {
152:     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
153:     uint dyadMinted = dyad.mintedDyad(address(this), id);
154:     Vault _vault = Vault(vault);
155:     uint value = amount * _vault.assetPrice() 
156:                   * 1e18 
157:                   / 10**_vault.oracle().decimals() 
158:                   / 10**_vault.asset().decimals();
159:  
160:     if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
161:     _vault.withdraw(id, to, amount);
162:     if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
163:   }

4. If the collateral ratio of the user falls below the minimum threshold of 1.5e18 (in terms of high volatility of collateral asset price), the attacker could then exploit the situation to liquidate the user using the liquidate() function.

Let's understand the second impact:

1. User tries to remove a vault by calling the remove() function.

2. Attacker frontruns the call by making a 1 wei collateral deposit through the deposit() function. This would increase the id2asset for the user in the vault.

File: VaultManagerV2.sol
127:   function deposit(
128:     uint    id,
129:     address vault,
130:     uint    amount
131:   ) 
132:     external 
133:       isValidDNft(id)
134:   {
135:     idToBlockOfLastDeposit[id] = block.number;
136:     Vault _vault = Vault(vault);
137:     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
138:     _vault.deposit(id, amount);
139:   }

3. User's call goes through and reverts due to the check on Line 102. This revert occurs since id2asset is now 1 wei for the vault the user is trying to remove. Note that although the attacker would be spending gas here, an equal amount of gas would also be required from the user's side to withdraw the 1 wei. The attack will continue till the user gives up due to the high gas spent behind withdrawing. Another thing to note is that regular users (with no knowledge of contracts) might not have the option to withdraw 1 wei from the frontend, which would require additional overhead from their side to seek help from the team.

File: VaultManagerV2.sol
095:   function remove(
096:       uint    id,
097:       address vault
098:   ) 
099:     external
100:       isDNftOwner(id)
101:   {
102:     if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
103:     if (!vaults[id].remove(vault))     revert VaultNotAdded();
104:     emit Removed(id, vault);
105:   }

Tools Used

Manual Review

Recommended Mitigation Steps

Use modifier isDNftOwner() instead of isValidDNft() on function deposit().

Assessed type

Invalid Validation

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

[L-03] withdraw() function does not consider kerosene value Kerosene value is not considered here in the check above the withdraw() call on the vault contract. This is problematic for kerosene vaults because getNonKeroseneValue() for users only using kerosene vaults would lead to revert due to 0 - non-zero amount occurring.

function withdraw( uint id, address vault, uint amount, address to ) public isDNftOwner(id) { if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock(); uint dyadMinted = dyad.mintedDyad(address(this), id); Vault _vault = Vault(vault); uint value = amount * _vault.assetPrice() * 1e18 / 10_vault.oracle().decimals() / 10_vault.asset().decimals();

if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
_vault.withdraw(id, to, amount);
if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 

}

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

[L-03] withdraw() function does not consider kerosene value Kerosene value is not considered here in the check above the withdraw() call on the vault contract. This is problematic for kerosene vaults because getNonKeroseneValue() for users only using kerosene vaults would lead to revert due to 0 - non-zero amount occurring.

function withdraw( uint id, address vault, uint amount, address to ) public isDNftOwner(id) { if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock(); uint dyadMinted = dyad.mintedDyad(address(this), id); Vault _vault = Vault(vault); uint value = amount * _vault.assetPrice() * 1e18 / 10_vault.oracle().decimals() / 10_vault.asset().decimals();

if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
_vault.withdraw(id, to, amount);
if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 

}

Lines of code

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

Vulnerability details

Impact

There is no incentive to liquidate low value positions such as 5$ USD worth of collateral because of gas costs on Ethereum (even worse in high gas scenarios). This causes the protocol to potentially accrue bad debt and become under-collaterized over time.

Proof of Concept

Liquidators will proceed to liquidate users if there is an incentive. If there isn't one, no one will call the liquidate() function.

For example, a user deposits 8 USD worth of collateral and mints 4 DYAD. Let's say the price of the collateral drops causing the value of go from 8 USD to 5 USD.

Now since the user has 5 USD worth of collateral and has 4 DYAD minted, the user is undercollateralized and must be liquidated in order to ensure that the protocol remains overcollateralized (150% of 4 DYAD = 6 USD minimum threshold required but value of collateral is 5 USD right now).

Because the value of the position is so low, after gas costs on Ethereum, liquidators will not make a profit liquidating this user. In the end, these low value positions will never get liquidated, leaving the protocol with bad debt and can even cause the protocol to be undercollateralized with enough small value accounts being underwater.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider allowing users to mint DYAD if their collateral value is past a certain threshold.

Assessed type

Error

Lines of code

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

Vulnerability details

Summary

Root cause: Function deposit() uses modifier isValidDNft() instead of isDNftOwner(), which allows anyone to call deposit() on behalf of any DNft id.

Impacts:

1. Attacker can make user devoid of withdrawing or redeeming collateral by making 0 value deposit() calls. Other than denying users from temporarily withdrawing their collateral, this is also an issue since it could force users into liquidations when they try to take preventative measures on their collateral ratio (especially through redeemDyad()) in high collateral price volatility situations. If successful, the attacker could then perform the liquidation to profit from the situation.
2. Attacker can make user devoid of removing vault due to id2asset > 0 by depositing 1 wei of collateral.

Proof of Concept

Let's understand the first impact:

1. User calls redeemDyad() (or withdraw() to directly withdraw collateral) to burn DYAD and withdraw their collateral asset.

2. Attacker frontruns the call with a 0 value deposit() call. This would set the idToBlockOfLastDeposit[id] to the current block.number. The call does not revert since 0 value transfers are allowed.

File: VaultManagerV2.sol
127:   function deposit(
128:     uint    id,
129:     address vault,
130:     uint    amount
131:   ) 
132:     external 
133:       isValidDNft(id)
134:   {
135:     idToBlockOfLastDeposit[id] = block.number;
136:     Vault _vault = Vault(vault);
137:     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
138:     _vault.deposit(id, amount);
139:   }

3. When the user's redeemDyad() call goes through, it internally calls the withdraw() function, which would cause a revert due to the check on Line 152. The check evaluates to true and reverts since the attacker changes the last deposit block number to the current block through the 0 value deposit() call.

File: VaultManagerV2.sol
143:   function withdraw(
144:     uint    id,
145:     address vault,
146:     uint    amount,
147:     address to
148:   ) 
149:     public
150:       isDNftOwner(id)
151:   {
152:     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
153:     uint dyadMinted = dyad.mintedDyad(address(this), id);
154:     Vault _vault = Vault(vault);
155:     uint value = amount * _vault.assetPrice() 
156:                   * 1e18 
157:                   / 10**_vault.oracle().decimals() 
158:                   / 10**_vault.asset().decimals();
159:  
160:     if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
161:     _vault.withdraw(id, to, amount);
162:     if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
163:   }

4. If the collateral ratio of the user falls below the minimum threshold of 1.5e18 (in terms of high volatility of collateral asset price), the attacker could then exploit the situation to liquidate the user using the liquidate() function.

Let's understand the second impact:

1. User tries to remove a vault by calling the remove() function.

2. Attacker frontruns the call by making a 1 wei collateral deposit through the deposit() function. This would increase the id2asset for the user in the vault.

File: VaultManagerV2.sol
127:   function deposit(
128:     uint    id,
129:     address vault,
130:     uint    amount
131:   ) 
132:     external 
133:       isValidDNft(id)
134:   {
135:     idToBlockOfLastDeposit[id] = block.number;
136:     Vault _vault = Vault(vault);
137:     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
138:     _vault.deposit(id, amount);
139:   }

3. User's call goes through and reverts due to the check on Line 102. This revert occurs since id2asset is now 1 wei for the vault the user is trying to remove. Note that although the attacker would be spending gas here, an equal amount of gas would also be required from the user's side to withdraw the 1 wei. The attack will continue till the user gives up due to the high gas spent behind withdrawing. Another thing to note is that regular users (with no knowledge of contracts) might not have the option to withdraw 1 wei from the frontend, which would require additional overhead from their side to seek help from the team.

File: VaultManagerV2.sol
095:   function remove(
096:       uint    id,
097:       address vault
098:   ) 
099:     external
100:       isDNftOwner(id)
101:   {
102:     if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
103:     if (!vaults[id].remove(vault))     revert VaultNotAdded();
104:     emit Removed(id, vault);
105:   }

Tools Used

Manual Review

Recommended Mitigation Steps

Use modifier isDNftOwner() instead of isValidDNft() on function deposit().

Assessed type

Invalid Validation

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

[L-04] Attacker can grief users from redeeming collateral The burnDyad() function allows anyone to burn DYAD on behalf of any id. This allows an attacker to burn DYAD and thus prevent users from possible redeeming their collateral back due to mintedDyad underflowing during redeem.

function burnDyad( uint id, uint amount ) external isValidDNft(id) { dyad.burn(id, msg.sender, amount); emit BurnDyad(id, amount, msg.sender); }