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: 15/183
Findings: 10
Award: $585.68
🌟 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
Currently Deploy script adds newly deployed WETH,wstETH vaults to VaultLicenser and KerosineManager. User can add the same vault twice to usual vaults
and to kerosineVaults
.
It is very easy to exploit: User adds the same vault twice and his deposit is twice valuable than it is. It allows to mint free DYAD which breaks the assumption that 1 DYAD is backed by at least 1 USD. As a result, protocols integrating DYAD can be exploited.
For example:
60,000 / 150% = 40,000 DYAD
In deploy script you can see that newly deployed WETH,wstETH vaults are added to VaultLicenser and KerosineManager:
function run() public returns (Contracts memory) { ... @> kerosineManager.add(address(ethVault)); @> kerosineManager.add(address(wstEth)); vaultManager.setKeroseneManager(kerosineManager); ... @> vaultLicenser.add(address(ethVault)); @> vaultLicenser.add(address(wstEth)); ... }
And here you can see that the same WETH,wstETH can be registered twice:
function add( uint id, address vault ) external isDNftOwner(id) { 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); } function addKerosene( uint id, address vault ) external isDNftOwner(id) { if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults(); @> if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed(); if (!vaultsKerosene[id].add(vault)) revert VaultAlreadyAdded(); emit Added(id, vault); }
Manual Review
Make two sets in KerosineManager:
Other
#0 - c4-pre-sort
2024-04-29T05:26:58Z
JustDravee marked the issue as duplicate of #966
#1 - c4-pre-sort
2024-04-29T08:38:08Z
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:25Z
koolexcrypto marked the issue as duplicate of #1133
#4 - c4-judge
2024-05-29T07:06:41Z
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
User can add Kerosine vault as usual exogenous vault via function add()
because in Deploy script it's mistakenly licensed.
It breaks the core invariant that every DYAD is backed by at least 1 USD of exogenous collateral, because now Kerosine can be used as single collateral
if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
Another impact is that UnboundedKerosineVault cannot be added in addKerosene()
because it's not added to KerosineManager in Deploy script.
In deploy script you can see that unboundedKerosineVault
is registered in vaultLicenser
:
function run() public returns (Contracts memory) { vm.startBroadcast(); // ---------------------- Licenser vaultLicenser = new Licenser(); // Vault Manager needs to be licensed through the Vault Manager Licenser VaultManagerV2 vaultManager = new VaultManagerV2( DNft(MAINNET_DNFT), Dyad(MAINNET_DYAD), vaultLicenser ); ... vaultLicenser.add(address(ethVault)); vaultLicenser.add(address(wstEth)); @> vaultLicenser.add(address(unboundedKerosineVault)); @> // vaultLicenser.add(address(boundedKerosineVault)); ... }
However vaultLicenser
is meant to contain exogenous vaults, so unboundedKerosineVault
can be used as exogenous vault:
function add( uint id, address vault ) external isDNftOwner(id) { 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); }
Manual Review
Do not license Kerosine vaults in VaultLicenser, instead license them in separate KerosineVaultLicenser.
Other
#0 - c4-pre-sort
2024-04-29T05:43:40Z
JustDravee marked the issue as duplicate of #966
#1 - c4-pre-sort
2024-04-29T08:38:08Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-04T09:46:29Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-28T15:28:27Z
koolexcrypto marked the issue as duplicate of #1133
#4 - c4-judge
2024-05-29T07:06:46Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: MrPotatoMagic
Also found by: 0xtankr, ArmedGoose, Egis_Security, Giorgio, KYP, Maroutis, NentoR, OMEN, Sabit, Shubham, SpicyMeatball, T1MOH, d3e4, dimulski, peanuts
200.8376 USDC - $200.84
Protocol uses only full liquidation, it doesn't allow to liquidate partially in comparison to usual lending protocols.
For example whale's position is very big and now becomes liquidateable. On liquidation whole position's DYAD debt must be repayed. To perform such a big liquidation, liquidator must borrow the same amount of DYAD, i.e. deposit the same amount as whale to protocol and repay on behalf of whale.
Usually flashloan is used to perform such liquidation, however in this protocol liquidator is unable to deposit and withdraw in the same block, so the only case for liquidator is to have free funds as much as whale's position to perform liquidation which reduces liquidation efficiency.
And this is where game theory comes into play: if liquidator has more funds than whale to perform that liquidation, then he becomes that non-liquidateable whale.
As a result, protocol will accumulate bad debt because of huge non-liquidateable position.
Here you can see that liquidator must repay liquidatee's whole debt.
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)); ... }
Here you can see that user can't withdraw at the same block as deposited last time. It means flashloan can't be used for liquidation
function withdraw( uint id, address vault, uint amount, address to ) public isDNftOwner(id) { @> if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock(); ... }
Manual Review
Allow partial liquidation.
Other
#0 - c4-pre-sort
2024-04-28T10:05:06Z
JustDravee marked the issue as duplicate of #1097
#1 - c4-pre-sort
2024-04-29T08:34:32Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-11T12:22:11Z
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
There is variable idToBlockOfLastDeposit
to prevent deposit and withdraw in the same block.
But attacker can frontrun user's withdrawal by depositing 1 wei to his Dnft. As a result, user's withdrawal will revert.
There is no limitation in this attack, attacker can block withdrawal for any user for any amount of time.
In deposit it only checks that DNft is minted, not only user can deposit to it
function deposit( uint id, address vault, uint amount ) external @> isValidDNft(id) { idToBlockOfLastDeposit[id] = block.number; ... } function withdraw( uint id, address vault, uint amount, address to ) public isDNftOwner(id) { if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock(); ... }
Manual Review
Allow only user to deposit to DNft:
function deposit( uint id, address vault, uint amount ) external - isValidDNft(id) + isDNftOwner(id) { ... }
DoS
#0 - c4-pre-sort
2024-04-27T11:56:40Z
JustDravee marked the issue as duplicate of #489
#1 - c4-pre-sort
2024-04-29T09:28:52Z
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:35Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#4 - c4-judge
2024-05-05T21:49:37Z
koolexcrypto marked the issue as nullified
#5 - c4-judge
2024-05-05T21:49:40Z
koolexcrypto marked the issue as not nullified
#6 - c4-judge
2024-05-08T15:26:42Z
koolexcrypto marked the issue as duplicate of #1001
#7 - c4-judge
2024-05-11T19:49:11Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: 0xAlix2
Also found by: 0x486776, 0xabhay, 0xlucky, 0xtankr, Abdessamed, Circolors, CodeWasp, DarkTower, Egis_Security, Giorgio, Infect3d, Krace, KupiaSec, Limbooo, Maroutis, NentoR, Ryonen, SpicyMeatball, T1MOH, TheFabled, TheSavageTeddy, TheSchnilch, VAD37, XDZIBECX, btk, carrotsmuggler, cu5t0mpeo, dimulski, gumgumzum, iamandreiski, imare, itsabinashb, ke1caM, kennedy1030, lian886, n4nika, oakcobalt, sashik_eth, shaflow2, steadyman, web3km, windhustler, zhaojohnson
3.8221 USDC - $3.82
Kerosine price is determined by formula price = (TVL - DyadSupply) / KerosineSupply
.
Where TVL according to docs:
C: the total USD value of all exogenous collateral in the protocol (TVL). Critically, this total does not include Kerosene itself
Old vaults contain in total 1.8M USD and currently DYAD supply is 600_000 tokens. Problem is that after deploying V2 it will account for DYAD's current supply, but not account TVL in V1. As a result it will heavily underestimate Kerosine price.
Also it introduces attack vector:
TVL is calculated by looping through kerosineManager
's vaults. Note that it uses current DYAD supply which can be manipulated via flashloan in V1. So during flashloan attack numerator
can be equal to 0.
function assetPrice() public view override returns (uint) { uint tvl; address[] memory vaults = kerosineManager.getVaults(); uint numberOfVaults = vaults.length; for (uint i = 0; i < numberOfVaults; i++) { @> Vault vault = Vault(vaults[i]); tvl += vault.asset().balanceOf(address(vault)) * vault.assetPrice() * 1e18 / (10**vault.asset().decimals()) / (10**vault.oracle().decimals()); } @> uint numerator = tvl - dyad.totalSupply(); uint denominator = kerosineDenominator.denominator(); return numerator * 1e8 / denominator; }
Old vaults are not added to KerosineManager in deploy script. It only adds newly deployed vaults:
KerosineManager kerosineManager = new KerosineManager(); kerosineManager.add(address(ethVault)); kerosineManager.add(address(wstEth)); vaultManager.setKeroseneManager(kerosineManager);
Manual Review
It must use only DYAD supply that is minted in V2 excluding tokens from V1.
Other
#0 - c4-pre-sort
2024-04-28T19:33:50Z
JustDravee marked the issue as duplicate of #308
#1 - c4-pre-sort
2024-04-29T09:05:13Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-11T20:08:36Z
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
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L221-L226
There are 2 type of vaults user can deposit into: usual Vault and KerosineVault. Usual Vault is expected to contain at least 100% of CR, i.e. at most 50% of CR can be kept in KerosineVaults (MCR is 150%).
Problem is that on liquidation it loops only through usual Vaults, it misses KerosineVaults. As a result, liquidator loses money on liquidation.
Suppose following scenario:
CR = 150%
. Liquidation bonus is 20%.uint cr = collatRatio(id); uint cappedCr = cr < 1e18 ? 1e18 : cr; uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr); 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); }
liquidationAssetShare = ((1.5 - 1) * 0.2 + 1) / 1.5 = 0.73
As a result, liquidator will pay 1000 USD in DYAD and receive only 1000 USD * 0.73 = 730 USD
.
Here you can see that it loops only through usual vaults: https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L221-L226
Manual Review
Loop through Kerosine Vaults too.
Other
#0 - c4-pre-sort
2024-04-28T10:20:36Z
JustDravee marked the issue as duplicate of #128
#1 - c4-pre-sort
2024-04-29T09:08:07Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-11T19:42:44Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: Infect3d
Also found by: 0x486776, 0xAlix2, 0xleadwizard, 0xnilay, Abdessamed, ArmedGoose, Bauchibred, Bigsam, GalloDaSballo, HChang26, Myrault, OMEN, SBSecurity, T1MOH, ZanyBonzy, alix40, atoko, iamandreiski, jesjupyter, ke1caM, miaowu, peanuts, vahdrak1
17.2908 USDC - $17.29
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L205-L228
Liquidation bonus is defined as 20% on excessive collateral of position. MCR is defined as 150%, i.e. liquidator receives at max 10% bonus of position amount.
Problem is that in case user's CR rapidly falls under 100%, liquidation is unprofitable because liquidator repays DYAD debt which is greater than collateral
As a result, protocol accumulates bad debt and DYAD can depeg.
Here you can see that 20% bonus is applied only for excessive collateral above 100% CR. So in case CR is less than 100%, liquidator pays more than receives:
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); } emit Liquidate(id, msg.sender, to); }
Finally if bad debt accumulates, TVL can become less than DYAD totalSupply. It will make all the withdrawals and liquidations revert because UnboundedKerosineVault.assetPrice()
will underflow:
function assetPrice() public view override returns (uint) { uint tvl; address[] memory vaults = kerosineManager.getVaults(); uint numberOfVaults = vaults.length; for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[i]); tvl += vault.asset().balanceOf(address(vault)) * vault.assetPrice() * 1e18 / (10**vault.asset().decimals()) / (10**vault.oracle().decimals()); } @> uint numerator = tvl - dyad.totalSupply(); ... }
Manual Review
Consider implementing mechanism for bad debt handling.
Other
#0 - c4-pre-sort
2024-04-28T10:10:40Z
JustDravee marked the issue as duplicate of #977
#1 - c4-pre-sort
2024-04-29T09:23:42Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-04T09:44:04Z
koolexcrypto changed the severity to QA (Quality Assurance)
#3 - c4-judge
2024-05-12T09:23:58Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#4 - c4-judge
2024-05-28T16:20:19Z
This previously downgraded issue has been upgraded by koolexcrypto
#5 - c4-judge
2024-05-28T16:21:49Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: Circolors
Also found by: 0xtankr, AamirMK, Al-Qa-qa, FastChecker, Infect3d, SBSecurity, Strausses, T1MOH, VAD37, carrotsmuggler, gumgumzum
122.4433 USDC - $122.44
BoundedKerosineVault has inner unboundedKerosineVault
from which he fetches Kerosine price.
Problem during deploy is that function setUnboundedKerosineVault()
is not called.
BoundedKerosineVault is meant to be added later. As a result there will be a problem if bounded vault is added as is: assetPrice()
will revert and therefore core function VaultManagerV2.getKeroseneValueI()
will revert. It will cause withdrawals and liquidations revert.
Here you can see that BoundedKerosineVault fetches price from UnboundedKerosineVault:
function setUnboundedKerosineVault( UnboundedKerosineVault _unboundedKerosineVault ) external onlyOwner { unboundedKerosineVault = _unboundedKerosineVault; } function assetPrice() public view override returns (uint) { return unboundedKerosineVault.assetPrice() * 2; }
However it doesn't call BoundedKerosineVault.setUnboundedKerosineVault()
in deploy script.
Manual Review
Set address in bounded vault:
BoundedKerosineVault boundedKerosineVault = new BoundedKerosineVault( vaultManager, Kerosine(MAINNET_KEROSENE), kerosineManager ); + boundedKerosineVault.setUnboundedKerosineVault(unboundedKerosineVault);
Other
#0 - c4-pre-sort
2024-04-29T07:37:31Z
JustDravee marked the issue as duplicate of #829
#1 - c4-pre-sort
2024-04-29T09:22:42Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-05T10:52:12Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-29T12:33:29Z
koolexcrypto marked the issue as satisfactory
🌟 Selected for report: dimulski
Also found by: 0xleadwizard, 0xlemon, Aamir, Al-Qa-qa, AvantGard, Bauchibred, Cryptor, DarkTower, Egis_Security, Giorgio, Maroutis, MrPotatoMagic, OMEN, Ocean_Sky, Ryonen, SBSecurity, Sabit, SpicyMeatball, Stefanov, T1MOH, Tigerfrake, WildSniper, atoko, bhilare_, darksnow, fandonov, grearlake, iamandreiski, igdbase, pontifex, web3km, xiao
4.8719 USDC - $4.87
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L205-L228
Liquidator bonus is defined as 20% on excessive collateral above 100% CR. This 20% bonus is measured in positions amount. Problem is that Liquidator is not incentivized to liquidate positions where liquidation bonus is less than gas costs for performing liquidation.
Here you can see that 20% liquidation bonus is applied on position size:
function liquidate( uint id, uint to ) external isValidDNft(id) isValidDNft(to) { ... 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); } emit Liquidate(id, msg.sender, to); }
Manual Review
Require min amount to be left on balance. Or introduce has stipend: fixed amount like 0.2 ETH which is deposited once and can be claimed by liquidator.
Other
#0 - c4-pre-sort
2024-04-29T07:52:27Z
JustDravee marked the issue as duplicate of #1258
#1 - c4-pre-sort
2024-04-29T09:16:29Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-03T14:07:47Z
koolexcrypto changed the severity to QA (Quality Assurance)
#3 - c4-judge
2024-05-22T14:26:07Z
This previously downgraded issue has been upgraded by koolexcrypto
#4 - c4-judge
2024-05-28T16:53:14Z
koolexcrypto marked the issue as satisfactory
#5 - c4-judge
2024-05-28T20:05:53Z
koolexcrypto marked the issue as duplicate of #175
🌟 Selected for report: Pataroff
Also found by: Egis_Security, Evo, Jorgect, MrPotatoMagic, SBSecurity, T1MOH, carrotsmuggler, ljj
223.9474 USDC - $223.95
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L177
VaultManagerV2 allows to burn DYAD for other position, it only checks that position exists.
Also when user redeems DYAD, it will revert on underflow in case user redeems more DYAD than his debt is. Combining these things attacker can frontrun users who redeem full debt by repaying 1 wei - users' transactions will fail.
Here you can see that user can repay debt on behalf of another valid position and it will underflow in dyad.burn()
:
function burnDyad( uint id, uint amount ) external @> isValidDNft(id) { dyad.burn(id, msg.sender, amount); emit BurnDyad(id, amount, msg.sender); } function burn( uint id, address from, uint amount ) external licensedVaultManager { _burn(from, amount); @> mintedDyad[msg.sender][id] -= amount; } function redeemDyad( uint id, address vault, uint amount, address to ) external isDNftOwner(id) returns (uint) { @> dyad.burn(id, msg.sender, amount); ... }
Manual Review
There are 2 options:
isDNftOwner(id)
in function burnDyad()
.redeemDyad()
to not underflow if specified amount to repay is greater than actual debt.DoS
#0 - c4-pre-sort
2024-04-28T05:31:38Z
JustDravee marked the issue as duplicate of #74
#1 - c4-pre-sort
2024-04-29T11:58:16Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-10T10:14:46Z
koolexcrypto marked the issue as duplicate of #992
#3 - c4-judge
2024-05-11T12:15:58Z
koolexcrypto marked the issue as satisfactory
#4 - c4-judge
2024-05-28T10:29:41Z
koolexcrypto marked the issue as duplicate of #100
🌟 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
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L50-L68
Firstly need to list formulas.
Kerosine price is determined as $Kp = \frac{TVL - DyadTotalSupply}{KerosineTotalSupply}$
Collateral Ratio is calculated as $CR = \frac{exogenousCollateralUSD + Kp * kerosineDeposit}{borrowedDyad}$
Here is attack idea with hypothetical numbers:
What is the core of manipulation? Whale can decrease Kerosine price by minting DYAD against his deposit: it increases DYAD supply making Kerosine price as low as third of previous value, it is because he can mint up to 2/3 of his deposited amount. And because it is whale, his deposited amount is close to TVL, so now we get different formula for Kerosine price at the end of manipulation: $X = \frac{WhaleDeposit - 2/3 * WhaleDeposit}{KerosinTotalSupply} = 1/3 * \frac{WhaleDeposit}{KerosinTotalSupply}$
Here is test with scenario described above. Profit in step 5 slightly differs, didn't investigated much. In test it says 1.01e24
instead of theoretical 0.72e24
.
// SPDX-License-Identifier: MIT pragma solidity =0.8.17; import "forge-std/Test.sol"; import "forge-std/console.sol"; import {DeployBase, Contracts} from "../script/deploy/DeployBase.s.sol"; import {Parameters} from "../src/params/Parameters.sol"; import {DNft} from "../src/core/DNft.sol"; import {Dyad} from "../src/core/Dyad.sol"; import {Licenser} from "../src/core/Licenser.sol"; import {VaultManager} from "../src/core/VaultManager.sol"; import {Vault} from "../src/core/Vault.sol"; import {Payments} from "../src/periphery/Payments.sol"; import {OracleMock} from "./OracleMock.sol"; import {ERC20Mock} from "./ERC20Mock.sol"; import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol"; import {ERC20} from "@solmate/src/tokens/ERC20.sol"; import "../src/staking/Kerosine.sol"; import "../src/core/KerosineManager.sol"; import "../src/core/VaultManagerV2.sol"; import "../src/core/Vault.kerosine.unbounded.sol"; contract CustomTest is Test { address user = address(1); address whale = address(2); Licenser licenser; Dyad dyad; Kerosine kerosine; KerosineManager kerosineManager; ERC20Mock usdc; Licenser vaultLicenser; DNft dnft; OracleMock usdcOracle; VaultManagerV2 vaultManagerV2; Vault usdcVault; UnboundedKerosineVault unboundedKerosineVault; KerosineDenominator kerosineDenominator; uint256 whaleId; uint256 userId; function setUp() public { // 1. Deploy licenser = new Licenser(); dyad = new Dyad(licenser); kerosine = Kerosine(address(new ERC20Mock("", ""))); kerosineManager = new KerosineManager(); usdc = new ERC20Mock("", ""); vaultLicenser = new Licenser(); dnft = new DNft(); usdcOracle = new OracleMock(1e8); vaultManagerV2 = new VaultManagerV2( dnft, dyad, vaultLicenser ); licenser.add(address(vaultManagerV2)); usdcVault = new Vault( vaultManagerV2, usdc, IAggregatorV3(address(usdcOracle)) ); kerosineManager.add(address(usdcVault)); vaultManagerV2.setKeroseneManager(kerosineManager); unboundedKerosineVault = new UnboundedKerosineVault( vaultManagerV2, kerosine, dyad, kerosineManager ); kerosineDenominator = new KerosineDenominator( kerosine ); unboundedKerosineVault.setDenominator(kerosineDenominator); vaultLicenser.add(address(usdcVault)); vaultLicenser.add(address(unboundedKerosineVault)); // 2. Set initial values // set Kerosine totalSupply ERC20Mock(payable(address(kerosine))).mint(user, 1e24); // mint USDC and DNft usdc.mint(whale, 100e24); usdc.mint(user, 1.4e24); whaleId = dnft.mintNft{value: 1 ether}(whale); userId = dnft.mintNft{value: 1 ether}(user); } function test_custom() public { // Step 2 vm.startPrank(whale); usdc.approve(address(vaultManagerV2), 100e24); vaultManagerV2.add(whaleId, address(usdcVault)); vaultManagerV2.deposit(whaleId, address(usdcVault), 100e24); vm.stopPrank(); // Step 3 vm.startPrank(user); usdc.approve(address(vaultManagerV2), 1.4e24); vaultManagerV2.add(userId, address(usdcVault)); vaultManagerV2.deposit(userId, address(usdcVault), 1.4e24); kerosine.approve(address(vaultManagerV2), 0.0025e24); vaultManagerV2.add(userId, address(unboundedKerosineVault)); vaultManagerV2.deposit(userId, address(unboundedKerosineVault), 0.0025e24); vaultManagerV2.mintDyad(userId, 1e24, user); vm.stopPrank(); console.log("User's collRatio after borrow: %e", vaultManagerV2.collatRatio(userId)); // Step 4 vm.prank(whale); vaultManagerV2.mintDyad(whaleId, 66.6e24, whale); // Step 5 vm.prank(whale); vaultManagerV2.liquidate(userId, whaleId); // Step 6 vm.startPrank(whale); vaultManagerV2.burnDyad(whaleId, dyad.balanceOf(whale)); uint256 usdcWhaleBalance = usdcVault.id2asset(whaleId); console.log("USDC balance of Whale: %e", usdcWhaleBalance); console.log("Whale's profit: %e", usdcWhaleBalance - 100e24); } }
Manual Review
I don't see easy mitigation to the issue, it can be design issue of Kerosine feature. Economical audit is heavily advised to ensure Kerosin feature doesn't introduce another flaws in design.
Other
#0 - c4-pre-sort
2024-04-28T06:10:59Z
JustDravee marked the issue as duplicate of #67
#1 - c4-pre-sort
2024-04-29T09:05:59Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-05T09:59:11Z
koolexcrypto changed the severity to 2 (Med Risk)
#3 - c4-judge
2024-05-08T11:50:06Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#4 - c4-judge
2024-05-08T12:57:50Z
koolexcrypto marked the issue as nullified
#5 - c4-judge
2024-05-08T12:57:54Z
koolexcrypto marked the issue as not nullified
#6 - c4-judge
2024-05-11T19:25:39Z
koolexcrypto marked the issue as satisfactory