DYAD - Evo's results

Lines of code

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

Vulnerability details

Impact

withdraw method can be blocked permanently for all ids for each block.

Proof of Concept

depsoit amount for a note id by design can't be withdrawn immediately, The user should wait for the next block to be able to withdraw his amount, Ethereum generate new block every 12 seconds, meaning the user who make a deposit he should wait at least a 12 seconds to be able to withdraw, which is on purpose as the protocol made this protection against flash loan attacks or any deposit and withdraw in one time transaction.

However, because of this condition idToBlockOfLastDeposit[id] = block.number, it can cause DoS and block the withdraw method, a simple scenario that Bob can call deposit method and pass any note id with 1 WEI of depsoit amount to any Vault:

A simple scenario:

• Bob pass Alex's note id which is 5.
• Bob deposit only 1 WEI
• Deposit succeed and block.number now stored in idToBlockOfLastDeposit at id 5.
• Alex call withdraw but revert is done idToBlockOfLastDeposit[id] == block.number
• Alex is blocked now and need to wait for the next block.

It seems a short time to wait, and Alex wasn't blocked for long time. However, this unfortunately could not be the case, Bob could use this bug to achieve a full block for many ids for many consecutive blocks.

The critical scenario:

• Bob will create a vault contract (BVault).
• Bob will create a contract with a method to call deposit with 1 WEI amount and pass his vault address within a loop of all note ids.

	for (uint i = 0; i < Ids; i++) {
		VaultManagerV2.deposit(Ids[i], BVault, 1 WEI);
	}

• Calls will be done to his vault since there is no check for licensed vault on deposit method.
• Withdrawals for all ids will be blocked on withdraw until the next block.
• Bob will repeat above for each next block.
• The protocol withdrawals will be blocked for next blocks until Bob stop.

This attack will cost nothing except the TX fee.

Tools Used

Manual Review

Recommended Mitigation Steps

• Remove the condition and use different implementation or keep the condition but allow the deposit only for valid nft owner.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

A revert will be done in withdraw when users try to withdraw from unbounded kerosene's vault, blocking the users from being able to withdraw their tokens.

Proof of Concept

Users will not be able to withdraw unbounded kerosene tokens, _vault.oracle().decimals() is being called in withdraw method but it seems there is no implementation of oracle in Vault.kerosine.unbounded.sol contract, according to the withdraw of unbounded kerosene, the transaction will revert at 10**_vault.oracle().decimals().

Since we can call withdraw for any vault address, if we call the method _vault.oracle().decimals() of deployed Vault.kerosine.unbounded.sol contract, an EVM revert will be done since no implementation exist, an example contract for implemented oracle, check Vault contract IAggregatorV3 public immutable oracle.

It applies on redeemDyad too since redeemDyad method is calling withdraw(id, vault, asset, to).

Tools Used

Manual review

Recommended Mitigation Steps

Add oracle variable to Vault.kerosine.unbounded.sol contract.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Users will not be able to withdraw kerosene tokens from unbounded kerosine vault incase his none kerosene value less than his kerosine amount.

Proof of Concept

Users can't withdraw kerosene tokens from Vault.kerosine.unbounded, withdraw method will check if (getNonKeroseneValue(id) - value < dyadMinted) otherwise revert, incase of unbounded koresene vault, the user will not be able to withdraw his kerosene tokens if he has no value of non-koresene collateral, which must not be a pre-condition for koresene.

Break down: Since the user didn't mint dyad, he should be able to withdraw koresene tokens, but according to the condition getNonKeroseneValue(id) - value < dyadMinted, getNonKeroseneValue will get the none kerosene value (eth,weth..), but the user might have zero value of it. so he will be blocked from withdrawing kerosene tokens.

Another case: A liquidator liquidated a user, since the user being liquidated he has less collateral value or none, but he still has kerosene tokens that he wants to withdraw, but he couldn't since his collateral value is less than his kerosene tokens in unbounded kerosene vault or has zero of it since a liquation was done and no remaining.

Note that getNonKeroseneValue is returning totalUsdValue for all note id's vaults, as the value is USD for note id collaterals, which will impact (getNonKeroseneValue(id) - value < dyadMinted condition.

Tools Used

Manual Review

Recommended Mitigation Steps

Exclude Kerosene unbounded vault from (getNonKeroseneValue(id) - value < dyadMinted condition.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

burnDyad could cause DoS for other users transactions when they attempt to burn their full amount of dyad.

Proof of Concept

burnDyad is allowing the user to burn dyad amounts for his note id and other users note ids, It seems a part of the protocol design that allow such a feature to make burn dyad availabe not only for self burn amount of mintedDyad, also for others.

After investigating the method, we can see that dyad.burn(id, msg.sender, amount) is being called from VaultManagerV2 address as the caller (msg.sender), now if we move further to burn method, we will find that _burn(from, amount) is burning Dyad ERC20 tokens from the user caller address, moving forward in the next line mintedDyad[msg.sender][id] -= amount it seems that an amount is being decreased from mintedDyad mapping for the passed note id.

The case of the issue: Supposed Bob would like to burn a 100 Dyad (his max Dyad minted amount) to achieve a withdraw for his collateral, Bob will call burn passing 100e18 amount as 100 of Dyad tokens, Alex will front-run Bob and call burn method, passing the note id of Bob (let's say id 8) with a minimum amount of 1 WEI Dyad , Bob's transaction will fail since his amount of mintedDyad become less than 100e18 and it will revert here mintedDyad[msg.sender][id] -= amount.

Since no check for isDNftOwner modifier on burn method, anyone can pass different note ids for other users and decrease their mintedDyad amount, this attack is cheap.

Tools Used

Manual Review

Recommended Mitigation Steps

Add isDNftOwner modifier on burn method, update other parts of the code accordingly.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

incorrect implementation of isLicensed method in Kerosine Manager.

Proof of Concept

Wrong implementation in KerosineManager.sol contract, isLicensed should call mapping (address => bool) public isLicensed to check if vault isLicensed true or false.

While return vaults.contains(vault) is checking if the vault exist in the vaults of EnumerableSet.AddressSet variable. In this case it's unable to make a vault in KerosineManager not Licensed, meaning the vault need to be removed to achieve not Licensed value.

isLicensed will return false;

Tools Used

Manual Review

Recommended Mitigation Steps

Change isLicensed method in KerosineManager to be like Licenser contract, or use isLicensed in Licenser contract.

Assessed type

Other