Platform: Code4rena
Start Date: 18/04/2024
Pot Size: $36,500 USDC
Total HM: 19
Participants: 183
Period: 7 days
Judge: Koolex
Id: 367
League: ETH
Rank: 49/183
Findings: 7
Award: $247.59
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Maroutis
Also found by: 0x486776, 0xShitgem, 0xabhay, 0xleadwizard, 0xlemon, 0xnilay, 0xtankr, 3docSec, AM, Aamir, Abdessamed, Al-Qa-qa, AlexCzm, Circolors, CodeWasp, Daniel526, Egis_Security, Emmanuel, Giorgio, Honour, Hueber, Infect3d, Krace, KupiaSec, LeoGold, Limbooo, PoeAudits, SBSecurity, SpicyMeatball, T1MOH, The-Seraphs, TheSavageTeddy, TheSchnilch, Topmark, VAD37, ZanyBonzy, adam-idarrha, bhilare_, btk, carlitox477, cinderblock, dimulski, falconhoof, grearlake, gumgumzum, iamandreiski, itsabinashb, josephdara, ke1caM, kennedy1030, ljj, n0kto, n4nika, neocrao, oakcobalt, petro_1912, pontifex, poslednaya, shaflow2, shikhar229169, web3km, ych18, zhaojohnson, zigtur
0.2831 USDC - $0.28
DYAD depeg and protocol failure is iminent.
VaultManagerV2.getNonKeroseneValue()
should return TVL of exogenous collateral (different forms of staked ETH) locked with nftID. Instead a bigger value can be returned,
because simple vaults
can contain kerosene vaults too.
dNFT owners can add 2 types of collateral vaults to mint DYAD: exogenous, which are ETH based vaults and endogenous collateral vaults (based on kerosene). There are 2 mappings that keep track of which vault is added to which nftId:
mapping(uint256 => EnumerableSet.AddressSet) internal vaults; mapping(uint256 => EnumerableSet.AddressSet) internal vaultsKerosene;
The problem lies in the fact users can add any type of vault to vaults
mapping, as long as the it's isLicensed
:
function add( uint id, address vault ) external isDNftOwner(id) { //@audit nft owners can add kerosene vaults to ETH-based vaults mapping if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults(); if (!vaultLicenser.isLicensed(vault)) revert VaultNotLicensed(); if (!vaults[id].add(vault)) revert VaultAlreadyAdded(); emit Added(id, vault); }
vaultLicenser
whitelists both kerosene and eth based vaults as we can see in DeployV2 script:
https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L93
... vaultLicenser.add(address(ethVault)); vaultLicenser.add(address(wstEth)); vaultLicenser.add(address(unboundedKerosineVault)); // vaultLicenser.add(address(boundedKerosineVault)); ...
getNonKeroseneValue
returns (eth + kerosene) value locked with respective nftId:
function getNonKeroseneValue( uint id ) public view returns (uint) { uint totalUsdValue; uint numberOfVaults = vaults[id].length(); for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[id].at(i)); uint usdValue; if (vaultLicenser.isLicensed(address(vault))) { // @audit return usdValue of all vaults: eth AND kerosene based usdValue = vault.getUsdValue(id); } totalUsdValue += usdValue; } return totalUsdValue; }
getNonKeroseneValue
is used in multiple places to ensure:
But since getNonKeroseneValue
returns an inflated value, both invariants are broken.
Manual review
KeroseneManager whitelists only exogenous collateral vaults. Add the following check to add
function to ensure only exogenous vaults can be added to vaults
mapping:
function add(uint id, address vault) external isDNftOwner(id) { if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults(); if (!vaultLicenser.isLicensed(vault)) revert VaultNotLicensed(); + if (keroseneManager.isLicensed(vault)) revert ExogenousVault(); if (!vaults[id].add(vault)) revert VaultAlreadyAdded(); emit Added(id, vault); } ## Assessed type Invalid Validation
#0 - c4-pre-sort
2024-04-29T06:38:22Z
JustDravee marked the issue as duplicate of #966
#1 - c4-pre-sort
2024-04-29T08:37:21Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-04T09:46:31Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-28T15:28:21Z
koolexcrypto marked the issue as duplicate of #1133
#4 - c4-judge
2024-05-29T14:03:07Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: Maroutis
Also found by: 0x486776, 0xShitgem, 0xabhay, 0xleadwizard, 0xlemon, 0xnilay, 0xtankr, 3docSec, AM, Aamir, Abdessamed, Al-Qa-qa, AlexCzm, Circolors, CodeWasp, Daniel526, Egis_Security, Emmanuel, Giorgio, Honour, Hueber, Infect3d, Krace, KupiaSec, LeoGold, Limbooo, PoeAudits, SBSecurity, SpicyMeatball, T1MOH, The-Seraphs, TheSavageTeddy, TheSchnilch, Topmark, VAD37, ZanyBonzy, adam-idarrha, bhilare_, btk, carlitox477, cinderblock, dimulski, falconhoof, grearlake, gumgumzum, iamandreiski, itsabinashb, josephdara, ke1caM, kennedy1030, ljj, n0kto, n4nika, neocrao, oakcobalt, petro_1912, pontifex, poslednaya, shaflow2, shikhar229169, web3km, ych18, zhaojohnson, zigtur
0.2831 USDC - $0.28
VaultManagerV2.getKeroseneValue
returns the wrong collateral value. Instead of returning the kerosene value locked with respective nftID, it returns the value of exogenous (wETH, wstETH) collateral locked.
DYAD can depegg from dollar.
dNFT owners can add 2 types of collateral vaults to mint DYAD: exogenous, which are ETH based vaults and endogenous collateral vaults (based on kerosene). These 2 mappings keep track of which vault is added to which nftId:
mapping(uint256 => EnumerableSet.AddressSet) internal vaults; mapping(uint256 => EnumerableSet.AddressSet) internal vaultsKerosene;
KeroseneManager role is to keep track of which vaults are used in Kerosene price calculation - only non-kerosene vaults are 'licensed'. https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L62-L65
//Deploy.V2.s.sol ... KerosineManager kerosineManager = new KerosineManager(); kerosineManager.add(address(ethVault)); kerosineManager.add(address(wstEth)); ...
The problem is that only keroseneManager
licensed vaults can be added to keroseneVaults
:
function addKerosene(uint id, address vault) external isDNftOwner(id) { if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults(); //@audit only ETH based vaults can be added if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed(); if (!vaultsKerosene[id].add(vault)) revert VaultAlreadyAdded(); emit Added(id, vault); }
This is problematic because getKeroseneValue
returns the value from vaultsKerosene
list, which, as we saw, are ETH based vaults.
Since totalUsdValue is calculated as keroseneValue + nonKeroseneValue, getNonKeroseneValue
will returns 2x nonKeroseneValue.
The TVL > DYAD total supply
invariant is broken because for each 100 USD worth of collateral deposited (wETH, wstETH), getTotalUsdValue
will returns twice the value. Users will be able to mint 2x more DYAD than they should be able to (ignoring the 150% overcollateralization which is making even worse).
Manual review
Consider the following updates:
function addKerosene( uint id, address vault ) external isDNftOwner(id) { if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults(); // ensure it's a valid vault + if (!vaultLicenser.isLicensed(vault)) revert VaultNotLicensed(); - if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed(); // ensure exogenous collateral vaults is not added + if (keroseneManager.isLicensed(vault)) revert ExogenousCollateral(); if (!vaultsKerosene[id].add(vault)) revert VaultAlreadyAdded(); emit Added(id, vault); } ... function getKeroseneValue( uint id ) public view returns (uint) { uint totalUsdValue; uint numberOfVaults = vaultsKerosene[id].length(); for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaultsKerosene[id].at(i)); uint usdValue; - if (keroseneManager.isLicensed(address(vault))) { // ensure vault is stil licensed by admins + if (vaultLicenser.isLicensed(address(vault))) { usdValue = vault.getUsdValue(id); } totalUsdValue += usdValue; } return totalUsdValue; ## Assessed type Invalid Validation
#0 - c4-pre-sort
2024-04-29T06:38:37Z
JustDravee marked the issue as duplicate of #966
#1 - c4-pre-sort
2024-04-29T08:37:25Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-04T09:46:30Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-28T15:28:22Z
koolexcrypto marked the issue as duplicate of #1133
#4 - c4-judge
2024-05-29T14:03:10Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: MrPotatoMagic
Also found by: 0x175, 0x486776, 0x77, 0xAkira, 0xAsen, 0xDemon, 0xabhay, 0xblack_bird, 0xlemon, 0xloscar01, 0xtankr, 3docSec, 4rdiii, Abdessamed, AlexCzm, Angry_Mustache_Man, BiasedMerc, Circolors, Cryptor, DMoore, DPS, DedOhWale, Dinesh11G, Dots, GalloDaSballo, Giorgio, Honour, Imp, Jorgect, Krace, KupiaSec, Mrxstrange, NentoR, Pechenite, PoeAudits, Ryonen, SBSecurity, Sabit, T1MOH, TheFabled, TheSavageTeddy, Tychai0s, VAD37, Vasquez, WildSniper, ZanyBonzy, adam-idarrha, alix40, asui, blutorque, btk, c0pp3rscr3w3r, caglankaan, carrotsmuggler, d_tony7470, dimulski, dinkras, djxploit, falconhoof, forgebyola, grearlake, imare, itsabinashb, josephdara, kartik_giri_47538, ke1caM, kennedy1030, koo, lionking927, ljj, niser93, pep7siup, poslednaya, ptsanev, sashik_eth, shaflow2, steadyman, turvy_fuzz, ubl4nk, valentin_s2304, web3km, xyz, y4y, zhaojohnson, zigtur
0.0234 USDC - $0.02
https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L127 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L143
Honest depositors can be denied from withdrawing their funds indefinitely.
VaultManagerV2.withdraw() can be easily DOSed. A malicious user can front-run a 'withdraw' request and deposit
1wei of collateral:
idToBlockOfLastDeposit
is set to current block.number;
https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L127
function deposit(uint256 id, address vault, uint256 amount) external isValidDNft(id) { idToBlockOfLastDeposit[id] = block.number; ...
Then, the honest withdraw
request is reverted if a deposit was done in same block.number:
https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L143
function withdraw(uint256 id, address vault, uint256 amount, address to) public isDNftOwner(id) { // @audit dos risc if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock(); ...
The attacker can continue this attack as long as he whishes (eg. waiting for collateral ratio to fall below liquidation threshold); The only cost incurred to bad actor is the gas fee, as deposit
doesn't enforce a minimum deposit.
Manual review
Consider allowing only nftOwner
to deposit under their nftID OR implement a mechanism to allow nftOwners to whitelists what addresses can deposit collateral under his nftID;
DoS
#0 - c4-pre-sort
2024-04-27T11:56:29Z
JustDravee marked the issue as duplicate of #489
#1 - c4-pre-sort
2024-04-29T09:25:44Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-05T20:39:59Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-05T20:45:36Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#4 - c4-judge
2024-05-05T21:52:07Z
koolexcrypto marked the issue as nullified
#5 - c4-judge
2024-05-05T21:52:10Z
koolexcrypto marked the issue as not nullified
#6 - c4-judge
2024-05-08T15:26:40Z
koolexcrypto marked the issue as duplicate of #1001
#7 - c4-judge
2024-05-11T19:49:07Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: Circolors
Also found by: 0x175, 0x486776, 0xAlix2, 0xSecuri, 0xShitgem, 0xfox, 0xlemon, 0xnilay, 3th, 4rdiii, Aamir, Al-Qa-qa, AlexCzm, Egis_Security, Evo, Honour, Infect3d, Josh4324, Limbooo, Mahmud, SBSecurity, TheSchnilch, ahmedaghadi, alix40, amaron, bbl4de, bhilare_, btk, carrotsmuggler, cinderblock, d3e4, dimulski, dinkras, ducanh2706, iamandreiski, itsabinashb, ke1caM, ljj, sashik_eth, shaflow2, steadyman, web3km, y4y
3.8221 USDC - $3.82
https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L146-L149 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L195-L198
Users are unable to withdraw kerosene tokens from unbounded kerosene vault due to a call to a non-existent method;
deposit and withdrawals of kerosene from the UnboundedKerosineVault
are executed through VaultManagerV2
because of the onlyVaultManager
modifier attached to both functions.
The vaultManager.withdraw()
function invokes several methods on the vault
argument: _vault.assetPrice()
, _vault.oracle()
, and _vault.asset()
. However, since the kerosene vaults lack an 'oracle' method, the transaction will fail, preventing users from withdrawing their kerosene tokens.
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() // @audit kerosene vault lask an oracle method; tx reverts / 10**_vault.asset().decimals(); ...
Manual review
For vaults
storage variable containing only exogenous collateral vaults (wETH, wstETH), consider updating following functions:
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(); + uint value = hasVault(id, vault) + ? amount * _vault.assetPrice() + * 1e18 + / 10**_vault.oracle().decimals() + / 10**_vault.asset().decimals() + : 0; // if kerosene vault, subtract 0 if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat(); _vault.withdraw(id, to, amount); if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); } function redeemDyad( uint id, address vault, uint amount, address to ) external isDNftOwner(id) returns (uint) { dyad.burn(id, msg.sender, amount); Vault _vault = Vault(vault); - uint asset = amount - * (10**(_vault.oracle().decimals() + -_vault.asset().decimals())) - / _vault.assetPrice() - / 1e18; + uint asset = hasVault(id, vault) + ? amount * (10**(_vault.oracle().decimals() + + _vault.asset().decimals())) + / _vault.assetPrice() + / 1e18 + : amount * 1e8/ _vault.assetPrice(); // kerosene vaults returns assetPrice() with 8 decimals precision withdraw(id, vault, asset, to); emit RedeemDyad(id, vault, amount, to); return asset; } - function hasVault(uint256 id, address vault) external view returns (bool) { + function hasVault(uint256 id, address vault) public view returns (bool) { return vaults[id].contains(vault); }
Invalid Validation
#0 - c4-pre-sort
2024-04-26T21:29:04Z
JustDravee marked the issue as duplicate of #1048
#1 - c4-pre-sort
2024-04-28T18:38:32Z
JustDravee marked the issue as duplicate of #830
#2 - c4-pre-sort
2024-04-29T08:44:53Z
JustDravee marked the issue as sufficient quality report
#3 - c4-judge
2024-05-11T20:04:17Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: 0xAlix2
Also found by: 0x175, 0x486776, 0xnev, 3docSec, 3th, Aamir, Abdessamed, AlexCzm, Angry_Mustache_Man, Circolors, DedOhWale, Emmanuel, Giorgio, Honour, Jorgect, KupiaSec, Maroutis, Myrault, SBSecurity, Stefanov, T1MOH, VAD37, Vasquez, adam-idarrha, alix40, ducanh2706, falconhoof, iamandreiski, ke1caM, kennedy1030, koo, lian886, ljj, miaowu, pontifex, sashik_eth, shikhar229169, vahdrak1
7.3026 USDC - $7.30
In some cases liquidators are getting back less than the value of burned DYAD making liquidations unprofitable and increasing the risk of protocol solvency.
Before delving into the issue, some context first.
The sponsor made it clear in the discord chat that :
- vaults are for exogenous collateral - kerosene vaults are for kerosene, the token, endogenous to the protocol
This means that dNFT owners can call vaultManagerV2's add
and addKerosene
to enable exogenous collateral (wETH, wstETH) respectively kerosene collateral deposits under their dNFT.
There is a problem that allow nftOwners to add any type of vault (eg. wETH vaults and kerosene vaults) to VaultManagerV2 vaults
mapping. This finding was reported in another issue and it's mentioned here only to give some more context.
After the above finding is fixed, in some conditions liquidations will be unprofitable.
In liquidate
a liquidationAssetShare
percentage is calculated. This represents the % of collateral liquidator should get: it includes the value of DYAD burned and a LIQUIDATION_REWARD
incentive bonus.
The problem is that only the exogenous collateral is moved to liquidator balance. In most cases the amounts received is smaller than the value of DYAD burned from their balance.
Let's take an example.
liquidate
will burn 100 DYAD and get back:liquidationAssetShare = ((cappedCr - 1e18) * LIQUIDATION_REWARD + 1e18) / cappedCr liquidationAssetShare = ((1.49 - 1) * 0.2 + 1) / 1.49 liquidationAssetShare = 1.098 / 1.49 ~= 0.7369
He will be rewarded with 73.69% of liquidated user collateral. But since vaults
contains only exogenous collateral, liquidator will receive
0.7369 * (100$ worth of wETH) = 73.69$ worth of wETH
which is smaller than the value of DYAD burned.
Consider looping over keroseneVauls as well and move corresponding % of kerosene to liquidator nftId:
function liquidate( uint id, uint to ) external isValidDNft(id) isValidDNft(to) { uint cr = collatRatio(id); if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh(); dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id)); uint cappedCr = cr < 1e18 ? 1e18 : cr; uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr); uint numberOfVaults = vaults[id].length(); for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[id].at(i)); uint collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare); vault.move(id, to, collateral); } + uint numberOfKeroseneVaults = vaultsKerosene[id].length(); + for (uint i = 0; i < numberOfKeroseneVaults; i++) { + Vault vault = Vault(vaults[id].at(i)); + uint collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare); + vault.move(id, to, collateral); } emit Liquidate(id, msg.sender, to); }
Invalid Validation
#0 - c4-pre-sort
2024-04-28T10:19:16Z
JustDravee marked the issue as duplicate of #128
#1 - c4-pre-sort
2024-04-29T09:03:39Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-11T19:42:55Z
koolexcrypto marked the issue as satisfactory
#3 - c4-judge
2024-05-13T18:40:17Z
koolexcrypto changed the severity to 3 (High Risk)
🌟 Selected for report: SBSecurity
Also found by: AlexCzm, Emmanuel, Stefanov, carlitox477, carrotsmuggler, d3e4, grearlake, peanuts
223.9474 USDC - $223.95
In case of a liquidation, liquidator receive less than the 20% bonus of the target Note’s backing colateral
.
According to documents, liquidator should receive a 20% bonus of the target note's backing collateral
on top of the DYAD burned.
Liquidation and Redemption
from docs :
If a Note’s collateral value in USD drops below 150% of its DYAD minted balance, it faces liquidation. The liquidator burns a quantity of DYAD equal to the target Note’s DYAD minted balance, and in return receives an equivalent value plus a 20% bonus of the target Note’s backing colateral, which the liquidator can direct to any other Note, usually their own. The target keeps the remainder of their collateral, if any.
No matter if the bonus of the target Note’s backing colateral
refers to the 150% overcollateralization target value or to the minimum of 100% colateral required to keep the DYAD peg, the liquidator receive less value than documents specify.
/// @inheritdoc IVaultManager function liquidate( uint id, uint to ) external isValidDNft(id) isValidDNft(to) { uint cr = collatRatio(id); if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh(); dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id)); uint cappedCr = cr < 1e18 ? 1e18 : cr; //@audit the 20% bonus is applied wrongly uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr); uint numberOfVaults = vaults[id].length(); for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[id].at(i)); uint collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare); vault.move(id, to, collateral); } emit Liquidate(id, msg.sender, to); }
Let's take s simple example to check the math.
liquidation
method:
-> he burns 100 DYAD
-> he will get back the following amount of collateral:
wethValueReceived = ((CR - 1e18) * LIQUIDATION_REWARD) + 1e18) / CR * CollateralValuewhere CR = capped collateral ratio = 1.49e18 CollateralValue = 149 USD
wethValueReceived = (1.49e18 - 1e18) * 0.2e18) + 1e18) / 1.49e18 * 149 wethValueReceived = 1.098 / 1.49 * 149 wethValueReceived = 0.7369127516778523 * 149 wethValueReceived = 109.8
No matter if target note's backing collateral
refers to minimum of collateral required to keep the DYAD peg position (eg. 100$ worth of collateral in above example) or the collateral amount required to keep the CR >= 150%, liquidator receive less than the 20% bonus.
The reward bonus percentage became smaller as the CR became smaller.
For a position of 100 DYAD minted and 120$ worth of collateral we get a CR = 1.2:
wethValueReceived = 1.04 / 1.2 * 120 = 104\$
received for burning 100 DYAD
That's a 4% bonus.
If the LIQUIDATION_REWARD bonus should be applied to the amount of DYAD burned (eg. for 100 DYAD repaid, liquidator should get 120% of that amount in collateral), consider updating the liquidation function:
/// @inheritdoc IVaultManager function liquidate( uint id, uint to ) external isValidDNft(id) isValidDNft(to) { uint cr = collatRatio(id); if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh(); dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id)); - uint cappedCr = cr < 1e18 ? 1e18 : cr; - uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); - uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr); // if CR <= 120% then liquidator get all his collateral + uint divisor = cr < 1.2e18 ? 1.2e18 : cr; + uint liquidationAssetShare = (1e18 + LIQUIDATION_REWARD) * 1e18 / divisor; uint numberOfVaults = vaults[id].length(); for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[id].at(i)); uint collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare); vault.move(id, to, collateral); } emit Liquidate(id, msg.sender, to); }
Math
#0 - c4-pre-sort
2024-04-29T06:39:41Z
JustDravee marked the issue as duplicate of #75
#1 - c4-pre-sort
2024-04-29T11:59:11Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-11T12:12:36Z
koolexcrypto marked the issue as satisfactory
#3 - c4-judge
2024-05-28T19:21:58Z
koolexcrypto marked the issue as duplicate of #982
🌟 Selected for report: TheSavageTeddy
Also found by: 0x175, 0x486776, 0xnev, AamirMK, AlexCzm, ArmedGoose, BiasedMerc, CaeraDenoir, Egis_Security, Jorgect, KYP, MrPotatoMagic, PoeAudits, SBSecurity, SovaSlava, VAD37, adam-idarrha, alix40, carrotsmuggler, d_tony7470, dimulski, grearlake, josephdara, ljj, n0kto, okolicodes, sashik_eth, sil3th, turvy_fuzz
7.3512 USDC - $7.35
https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L101 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L113 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L119-L131
Nft owners can be blocked from removing the vaults associated with their dNFT. Since they can add a limited number of vaults ((MAX_VAULTS = MAX_VAULTS_KEROSENE = 5), they can't add additional vaults if they reached max_vaults.
An attacker can front run remove
/ removeKerosene
requests and deposit
1wei of asset to the vault to be removed. Vault balance is increased:
https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.sol#L42-L51
// Vault.sol function deposit( uint id, uint amount ) external onlyVaultManager { id2asset[id] += amount; emit Deposit(id, amount); }
Because of following check the vault removal will revert: https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L94C1-L101C64
//VaultManagerV2.sol function remove( uint id, address vault ) external isDNftOwner(id) { if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
dNFT owner can't remove the vault. This can impact the users in different ways:
Due to 'function of the protocol or its availability' is impacted I consider M severity to be fair.
Manual review
Consider removing the id2asset(id) > 0
check and instead revert the tx if removal of vault makes the collateralRatio fall falls below threshold.
DoS
#0 - c4-pre-sort
2024-04-27T13:34:10Z
JustDravee marked the issue as duplicate of #489
#1 - c4-pre-sort
2024-04-29T09:25:44Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-05T20:39:59Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-05T20:45:36Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#4 - c4-judge
2024-05-05T21:51:34Z
koolexcrypto marked the issue as nullified
#5 - c4-judge
2024-05-05T21:51:38Z
koolexcrypto marked the issue as not nullified
#6 - c4-judge
2024-05-05T21:51:43Z
koolexcrypto marked the issue as not a duplicate
#7 - c4-judge
2024-05-06T08:56:26Z
koolexcrypto marked the issue as duplicate of #118
#8 - c4-judge
2024-05-11T12:23:40Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: carrotsmuggler
Also found by: 0xAlix2, 0xSecuri, 0xblack_bird, 0xnev, AM, Al-Qa-qa, AlexCzm, Dudex_2004, Egis_Security, GalloDaSballo, Infect3d, Jorgect, KupiaSec, Ryonen, SpicyMeatball, T1MOH, VAD37, adam-idarrha, amaron, cu5t0mpeo, d3e4, darksnow, forgebyola, foxb868, itsabinashb, jesjupyter, nnez, peanuts, pontifex, wangxx2026, windhustler, zhuying
4.8719 USDC - $4.87
Bad actors can deposit exogenous collateral but mint no DYAD to maximize the kerosene price increase. Users will mint DYAD using kerosene at inflated price; When the exogenous collateral is removed, kerosene price decreases and collateral ratio will fall drastically, exposing users to liquidation risk.
As mentioned in docs, Kerosene price is not set by market but has a deterministic value given by formula:
$K_{\text{value}} = \frac{TVL - D_{\text{supply}}}{K_{\text{supply}}}$
where: $K_{\text{value}}$ -> kerosene price TVL -> total value of exogenous collateral locked in the protocol $D_{\text{supply}}$ -> total DYAD minted $K_{\text{supply}}$ -> circulating kerosene supply
Kerosene price formula starts from the assumption that all exogenous collateral (eg. wETH) locked in vaults ensure all DYAD open positions stability (by a 'DYAD open position' I mean the total DYAD minted through a dNFT id).
A malicious user can deposit exogenous collateral (let's say wETH) into a dNFT for the only purpose to increase kerosene price. When another user deposit kerosene as collateral to mint DYAD, malicious user will back-run it and withdraw the wETH. Kerosene price drops and liquidation likelihood is increased.
Let's take the following example. For the simplicity, the exogenous collateral (eg. wETH) volatility is ignored.
1.Initial conditions TVL = $1M in wETH DYAD supply = 600k; Kerosene circulating supply(let's note it Ksupply) = 50M
Alice, a malicious user, deposit $400k in wETH
Bob, our victim, deposit $100k in wETH The new TVL = 1000k + 400k + 100k = 1500k Kerosene price = (TVL - DYAD supply) / Ksupply = (1500k - 500k) / 50M = $0.02
Bob deposit 5M kerosene, worth $100k
Bob mint 100k DYAD Kerosene price = (TVL - (DYAD supply + DYAD to mint)) / Ksupply kerosene price = (1500k - 700k) / 50M = $0.016 Bob's CollateralRatio = 100k + (0.016 * 5M) / 100k = 180% (a relatively comfortable overcolateralization)
Alice withdraw her wETH deposit TVL = 1500k - 400k = 1100k Kerosene price = (1100k - 700k) / 50M = $\0.008
Bob's CR = (100K + 0.008 * 5M) / 100k = 140% < MIN_COLLATERIZATION_RATIO = 150% Bob's position can be liquidated.
The same mechanism can be used for different purposes. Eg. front-run a liquidation tx and deposit exogenous collateral into a different dNFT he owns. Kerosene price is increased and user avoid being liquidated even if he did not deposited more collateral to his position.
If we check the deployed contracts we can see that the numbers from this example are similar to values from deployed contracts:
MAINNET_WETH_VAULT -> $1.22 M worth of wETH https://etherscan.io/address/0xcF97cEc1907CcF9d4A0DC4F492A3448eFc744F6c
MAINNET_KEROSENE -> 1_000_000_000 total supply, ~950M hold by MAINNET_OWNER => 50M in circulation https://etherscan.io/token/0xf3768D6e78E65FC64b8F12ffc824452130BD5394#readContract
MAINNET_DYAD -> 632k DYAD https://etherscan.io/token/0x305B58c5F6B5b6606fb13edD11FbDD5e532d5A26
Manual review
I see this issue as a design flaw. Users can abuse it with the only down side to lock exogenous collateral for 1 block (+gas fees).
An ideea to mitigate this price manipulation is locking the collateral for more than one block. Bad actors will not be able to decrease kerosene price on request, unless X block have passed since deposit.
MEV
#0 - c4-pre-sort
2024-04-28T05:56:22Z
JustDravee marked the issue as duplicate of #67
#1 - c4-pre-sort
2024-04-29T09:06:17Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-08T11:50:07Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-08T12:51:16Z
koolexcrypto marked the issue as satisfactory