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: 122/183
Findings: 2
Award: $7.32
π Selected for report: 0
π Solo Findings: 0
π 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#L119-L131 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L134-L153
Denial of Service for the Victim User of withdraw
& redeemDyad
functions of VaultManagerV2.sol .
The current implementation of VaultManagerV2.sol#withdraw
function has the following check before withdrawal from Vault where the state idToBlockOfLastDeposit
is being checked as shown below :
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L134C1-L153C4
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(); }
This check is implemented to prevent flashloan attacks . But the problem arises when a Malicious user can simply deposit a small amount of token to the Vault to prevent the withdrawal process . This arises due to the fact that the VaultManagerV2.sol#deposit
does not have a strict access modifier to prevent such attacks . Let's see how it can be executed . As shown below the VaultManagerV2.sol#deposit
function has a lineant function access modifier which just checks whether the dnft
used exists or not . And the function has this line of code where it changes the state idToBlockOfLastDeposit
to current block.number as shown below.
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L119C2-L131C4
function deposit( uint id, address vault, uint amount ) external isValidDNft(id) <@audit { idToBlockOfLastDeposit[id] = block.number; <@audit Vault _vault = Vault(vault); _vault.asset().safeTransferFrom(msg.sender, address(vault), amount); _vault.deposit(id, amount); }
The isValidDNft
is shown below :
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L42C1-L44C4
modifier isValidDNft(uint id) { if (dNft.ownerOf(id) == address(0)) revert InvalidDNft(); _; }
So whenever a Victim User tries to withdraw the asset's from the Vault , a Malicious User can frontrun the transaction and deposit 1 wei of token to the Vault to change the idToBlockOfLastDeposit
to current block.number , thereby preventing the Victim User from withdrawing the funds .
Manual Review
To prevent this attack the access modifier for the deposit function should be made more strict . Either the access modifier should be changed from isValidDNft
to isDNftOwner
or the User should be allowed to create a allowlist of Trusted Users , who would be allowed to deposit on behalf of the DNFTOwner
.
Access Control
#0 - c4-pre-sort
2024-04-27T11:54:07Z
JustDravee marked the issue as duplicate of #489
#1 - c4-pre-sort
2024-04-29T09:25:41Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-05T20:38:19Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-05T20:39:24Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#4 - c4-judge
2024-05-05T20:39:38Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#5 - c4-judge
2024-05-05T21:42:49Z
koolexcrypto marked the issue as nullified
#6 - c4-judge
2024-05-05T21:42:58Z
koolexcrypto marked the issue as not nullified
#7 - c4-judge
2024-05-08T15:27:55Z
koolexcrypto marked the issue as duplicate of #1001
#8 - c4-judge
2024-05-11T19:49:47Z
koolexcrypto marked the issue as satisfactory
#9 - c4-judge
2024-05-13T18:34:30Z
koolexcrypto changed the severity to 3 (High Risk)
π 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/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L205-L228 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L34-L35
Detailed description of the impact of this finding.
Let's see the implementation of the VaultManagerV2.sol#liquidate
to see the calculation of the collateral earned by the Liquidator upon liquidation .
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L205C2-L228C4
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(); <@audit for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[id].at(i)); <@audit uint collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare); <@audit vault.move(id, to, collateral); <@audit } emit Liquidate(id, msg.sender, to); }
Here we can see that only the non-kerosene vaults of the DNFT's are considered . The kerosene vaults represented as vaultsKerosene
is not considered . Check this code below where both kerosene and non-kerosene vaults are declared at the beginning of the code :
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L34C1-L36C1
mapping (uint => EnumerableSet.AddressSet) internal vaults; mapping (uint => EnumerableSet.AddressSet) internal vaultsKerosene;
As per the official documentation given in the contest ReadMe , the liquidator must be given all the collateral including the collateral from kerosene vaults as shown below in the doc - Check the lines under the section - Liquidation and Redemption
for the same :
https://dyadstable.notion.site/DYAD-design-outline-v5-ed2d075eb691482d90cae262f822a2ff
The liquidating Note absorbs the liquidated Noteβs DYAD balance and captures its collateral, including Kerosene.
Let's take an example to understand the situation .
The MIN_COLLATERIZATION_RATIO = 1.5e18; // 150% in the protocol , let's say User has 160 USD worth of collateral in both Non-Kerosene & Kerosene Vaults and has minted 100 USD worth of dyad . Which implies the User to have collaterization ratio above 1.5e18 . But due to varying market conditions the value of collateral fell to 120 USD worth of collateral in Vaults where 80 USD worth of collateral was in Non-Kerosene Vault and 40 USD worth of collateral was in Kerosene Vaults . Now as the collaterization ratio fell below the minimum collaterization mark , the Liquidator comes to Liquidate the User and burns his 100 USD worth of DYAD for the same . The issue occurs as the Liquidator gets paid his reward only from Non-Kerosene
vaults and not from Kerosene
Vaults , as they were not included in the code implementation as shown above , causing heavy loses to the Liquidator .
Manual Review
Include Kerosene vaults in the calculation of collateral given to the Liquidator and move both Non-Kerosene & Kerosene Vaults colllateral to Liquidator's id to prevent losses for the Liquidator .
Math
#0 - c4-pre-sort
2024-04-28T10:23:53Z
JustDravee marked the issue as duplicate of #128
#1 - c4-pre-sort
2024-04-29T09:03:38Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-11T19:42:59Z
koolexcrypto marked the issue as satisfactory