DYAD - Infect3d's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/VaultManagerV2.sol#L250-L286 https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/script/deploy/Deploy.V2.s.sol#L95

Vulnerability details

Vulnerability details

DYAD protocol allow users to deposit collateral and mint a dollar-pegged asset (DYAD) against it. Two types of vaults are proposed:

• exogenous vaults, where users can deposit assets external to the protocol (like WETH, wstETH, ...)
• kerosine vaults, where user can deposit the endogenous token from the protocol, Kerosine

The issue is that right now, the deploy script declare WETH and wstETH vaults as both being exogenous and kerosine vaults. Also, unboundedKerosineVault is licensed in Licenser while it should be in KerosineManager This completely mess-up the collatral accounting.

https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/script/deploy/Deploy.V2.s.sol#L64-L65

File: script/deploy/Deploy.V2.s.sol
61: 
62:     KerosineManager kerosineManager = new KerosineManager();
63: 
64:❌	kerosineManager.add(address(ethVault)); //<@audit: should not be added in KerosineManager
65:❌	kerosineManager.add(address(wstEth));	//...

https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/script/deploy/Deploy.V2.s.sol#L95

File: script/deploy/Deploy.V2.s.sol
93:     vaultLicenser.add(address(ethVault));
94:     vaultLicenser.add(address(wstEth));
95:❌	vaultLicenser.add(address(unboundedKerosineVault)); //<@audit: should be added in KerosineManager, not Licenser
96:     // vaultLicenser.add(address(boundedKerosineVault));

This also make it possible for a user to add these vaults both to their vaults and vaultsKerosene enumerableSet.

Which means that when collateral ratio will be estimated, user WETH and wstETH deposit will be counted as Kerosene and non-Kerosine value, effectively double-counting this collateral value:

File: src/core/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); //<@audit: actual deployment script will cause issue here
248:   }

Impact

Loss of funds, user will be able to inflate their collateral value and thus mint more DYAD than system should allow

Tools Used

Manual review

Recommended Mitigation Steps

The following diff will not be sufficient to make the whole system function correctly, as there are an issue with how Kerosine price calculate TVL. Right now, the KerosineManager vaults set holds two functions:

• list licensed kerosine vault
• list assets that are part of the TVL

These two functions have no link, and shouldn't be mixed in the same set. Sponsor also refactor this part and separate this as two sets.

File: script/deploy/Deploy.V2.s.sol:64

    KerosineManager kerosineManager = new KerosineManager();

-   kerosineManager.add(address(ethVault));
-   kerosineManager.add(address(wstEth));
    vaultManager.setKeroseneManager(kerosineManager);

    kerosineManager.transferOwnership(MAINNET_OWNER);

    UnboundedKerosineVault unboundedKerosineVault = new UnboundedKerosineVault(
      vaultManager,
      Kerosine(MAINNET_KEROSENE), 
      Dyad    (MAINNET_DYAD),
      kerosineManager
    );

    BoundedKerosineVault boundedKerosineVault     = new BoundedKerosineVault(
      vaultManager,
      Kerosine(MAINNET_KEROSENE), 
      kerosineManager
    );

    KerosineDenominator kerosineDenominator       = new KerosineDenominator(
      Kerosine(MAINNET_KEROSENE)
    );

    unboundedKerosineVault.setDenominator(kerosineDenominator);

    unboundedKerosineVault.transferOwnership(MAINNET_OWNER);
    boundedKerosineVault.  transferOwnership(MAINNET_OWNER);

    vaultLicenser.add(address(ethVault));
    vaultLicenser.add(address(wstEth));
-   vaultLicenser.add(address(unboundedKerosineVault));
+   kerosineManager.add(address(unboundedKerosineVault));
-   // vaultLicenser.add(address(boundedKerosineVault));
+   kerosineManager.add(address(boundedKerosineVault));

Assessed type

Context

Lines of code

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

Vulnerability details

Vulnerability details

To prevent users from exploiting oracle updates for arbitrage, a safeguard has been implemented. This safeguard makes it impossible to deposit and then withdraw in the same block number.

But there 3 issues how this is implemented in VaultManagerV2::deposit:

1. No check that vault is licensed
2. Combined with the fact that user1 can deposit in user2 dNFT
3. Depositing in vault1 for a dNFT block withdrawals for all vault of this dNFT

This make it possible for a malicious user to call deposit with a empty dummy vault, which will lock withdrawal for the victim for the present block for all vaults Even with no dummy vault, a malicious user could deposit dust amount to any of the victim vaults to prevent him from withdrawing in same block

Impact

Possibility to prevent user from withdrawing

Proof of Concept

File: src/core/VaultManagerV2.sol
119:   function deposit(
120:     uint    id,
121:     address vault,
122:     uint    amount
123:   ) 
124:	external 
125:❌ 	isValidDNft(id) //<(1): missing isLicensed(vault) modifier
126:   {
127:❌	idToBlockOfLastDeposit[id] = block.number; //<(3): locking withdrawal for all vaults for this id
128:    Vault _vault = Vault(vault);
129:    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
130:    _vault.deposit(id, amount);
131:   }
132: 
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(); //<(3)
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:   }

Tools Used

manual review

Recommended Mitigation Steps

1. Verify vault is licensed (could also use the unused hasVault which might be even better)
2. Depositing on behalf of someone else should be permissioned, for example by implementing a permission mechanism: mapping(address owner => address) allowed + modifier isAllowed(address))

: src/core/VaultManagerV2.sol:119

+  mapping(address owner => address) allowed;
   function deposit(
     uint    id,
     address vault,
     uint    amount
   ) 
     external 
	isValidDNft(id)
+	isLicensed(vault)
+	isAllowed(msg.sender)
   {
     idToBlockOfLastDeposit[id] = block.number;
     Vault _vault = Vault(vault);
     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
     _vault.deposit(id, amount);
   }

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

In order to withdraw assets from a vault, the collateral ratio (CR) of the position must be sufficient :

    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 

The same checks are made in liquidate and mintDyad

The thing is, VaultManagerV2::collatRatio calls VaultManagerV2::getTotalUsdValue, which calls VaultManagerV2::getKeroseneValue which calls KerosineVault::getUsdValue ultimately calling UnboundedKerosineVault::assetPrice()

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

But neither this contract, or KerosineVault (which it inherit from) has an oracle getter to call.

Impact

Functions calling collatRatio() are: liquidate, withdraw and mintDyad This means user depositing Kerosine will see all their asset stuck in the vault. But not only that, positions cannot be liquidated until the problem is solved (by redeploying a new KerosineVault with an oracle)

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

Add an oracle to the KerosineVault (or UnboundedKerosineVault)

abstract contract KerosineVault is IVault, Owned(msg.sender) {
	using SafeTransferLib for ERC20;

	IVaultManager   public immutable vaultManager;
	ERC20           public immutable asset;
	KerosineManager public immutable kerosineManager; //@audit unused state variable
+	IAggregatorV3 public immutable oracle;

	mapping(uint => uint) public id2asset;

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

DYAD protocol allow users to deposit collateral and mint a dollar-pegged asset (DYAD) against it. Two types of vaults are proposed:

• exogenous vaults, where users can deposit assets external to the protocol (like WETH, wstETH, ...)
• kerosine vaults, where user can deposit the endogenous token from the protocol, Kerosine

Minting of DYAD tokens require a minimum total (exo + endo) collateralization ratio >150%, and a minimum exo CR100%. So this assertion should always be verified under good conditions (i.e prices do not changes to quickly): tvl > dyad.totalSupply()

But this is not a realistic that can hold forever.

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

File: src/core/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();//<@audit: this can underflow
66:       uint denominator = kerosineDenominator.denominator();
67:       return numerator * 1e8 / denominator;
68:   }
69: }

But if multiple vaults, or a major vault sees its asset price plummet, this relationship will change. In that case, the substraction will underflow resulting in a revert of assetPrice

The issue is that UnboundedKerosineVault::assetPrice is called by VaultManagerV2::collatRatio >> VaultManagerV2::getTotalUsdValue >> VaultManagerV2::getNonKeroseneValue And VaultManagerV2::collatRatio is called by VaultManagerV2::liquidate, which is the mechanism expected to clean up bad positions.

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

File: src/core/VaultManagerV2.sol
205:   function liquidate(
206:     uint id, //The ID of the dNFT to be liquidated.
207:     uint to //The address where the collateral will be sent
208:   ) 
209:     external 
210:       isValidDNft(id)
211:       isValidDNft(to)
212:     {
213:❌	   uint cr = collatRatio(id); //<@audit: this can revert if TVL < dyad.totalSupply()
214:       if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
215:       dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));
216: 

Impact

If total TVL drops below the minimum CR of the protocol, liquidation will revert, preventing protocol to recover.

Tools Used

Manuel review

Recommended Mitigation Steps

Mitigating that issue is difficult as the issue is associated to how Kerosine price is evaluated. This require to refactor how the token is priced, in case of such cases, but this surely shouldn't revert when called during liquidations. Collateralization ratio should also probably be revised to higher values to absorb unexpected dumps in value.

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

Right now there are no incentives to liquidate a position with a CR<1, as the liquidator will have to burn the full borrowed amount, and will get the full collateral.

But a CR<1 means collateral is worth less than borrowed amount.

So this is a clear loss for the liquidator, meaning no-one will liquidate the position.

File: src/core/VaultManagerV2.sol
205:   function liquidate( 
206:     uint id, //The ID of the dNFT to be liquidated.
207:     uint to //The address where the collateral will be sent
208:   ) 
...: 	// ... some code ...
215:❌	   dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id)); //<@audit: caller need to burn full borrowed amount
216: 
217:       uint cappedCr               = cr < 1e18 ? 1e18 : cr; /// == max(1e18, cr)
218:       uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); /// if cr<1, this is equal 0
219:       uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr); /// if cr<1, this is equal 1e18
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:   }

Impact

If CR<1, position will not be liquidated, protocol possibly incuring a worser bad debt than this could have been.

Tools Used

Manuel review

Recommended Mitigation Steps

Refactor the liquidation calculation, to allow liquidator to repay the debt and still get a reward out of this.

Assessed type

Context

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

L04 - Missing setUnboundedKerosenVault initialization in deploy https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/script/deploy/Deploy.V2.s.sol#L89-L89

Vulnerability details boundedKerosineVault is deployed, but the setUnboundedKerosenVault is not called, which will cause a revert when BounderKerosineVault::assetPrice() will be called to price users kerosine collateral:

File: src/core/Vault.kerosine.bounded.sol 22: 23: function setUnboundedKerosineVault( 24: UnboundedKerosineVault _unboundedKerosineVault 25: ) 26: external 27: onlyOwner 28: { 29: unboundedKerosineVault = _unboundedKerosineVault; 30: } 31: ...: ...: /// ... some code ... ...: 44: function assetPrice() 45: public 46: view 47: override 48: returns (uint) { 49: return unboundedKerosineVault.assetPrice() * 2; 50: }

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/Vault.kerosine.unbounded.sol#L65-L66 https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/staking/KerosineDenominator.sol#L17-L22

Vulnerability details

Vulnerability details

The protocol sets Kerosene’s value as DYAD collateral deterministically (documentation):

The implementation can be found here: https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/Vault.kerosine.unbounded.sol#L65-L66

File: src/core/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 price can easily be manipulated by an malicious actor (Alice) with sufficient funds:

1. Alice deposit the assets into its dNFT position
2. tvl will increase, dyad.totalSupply() will stay the same, so numerator will increase, thus Kerosene price
3. denominator will not change
4. Now Alice waits for victims to use Kerosine as collateral for their borrow
5. Once Alice see enough interesting positions, she mints DYAD, this will increase dyad.totalSupply(), thus reducing numerator and so Kerosine price
6. Now, multiple position will become liquidatable (CR < MIN_COLLATERIZATION_RATIO)
7. It become even worse now, because each liquidation will reduce dyad.totalSupply(), causing a liquidation cascade and crashing Kerosine price.

Please note that this effect is proportional to the supply controlled by the whales, but any amount used to operate that exploit will manipulate the price proportionally.

Impact

Unfair liquidation and Kerosine price crash

Proof of Concept

I have setup a test environment based on existing VaultManager.t.sol and DeployBase.s.sol, but had to make some additions to run the tests against VaultManagerV2 Thus will have to add these two files VaultManagerV2.t.sol and DeloyBaseV2.sol, available in the gist that follows: https://gist.github.com/InfectedIsm/111d71fb5265a1f62e3a248c64d5afa0 These files must be added at the same level as VaultManager.t.sol and DeployBase.s.sol respectively.

Output of the test:

Ran 1 test for test/VaultManagerV2.t.sol:VaultManagerV2Test
[PASS] test_ManipulatePrice() (gas: 1046778)
Logs:
  ker price: 150000
  ker price: 100000
  price change: -33%

Tools Used

Manual review

Recommended Mitigation Steps

Do not use spot value to calculate Kerosine price, but rather an oracle (can be a TWAP based on historic balance/supply) The longer the TWAP window will be chosen, the smoother the price change will be, allowing users to anticipate the manipulation and act accordingly on their position.

Assessed type

MEV