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: 92/183
Findings: 3
Award: $21.39
π 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
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L80-L91 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.bounded.sol#L44-L50 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L64-L65
The deployment script improperly configures KerosineManager
by registering non-kerosene vaults (ethVault
and wstEth
) instead of the intended kerosene-related vaults (UnboundedKerosineVault
and BoundedKerosineVault
). This misconfiguration leads to multiple functional issues:
Addition of Kerosene Vaults:
The addKerosene
function in VaultManagerV2
, dependent on KerosineManager
for licensing verification, fails because the necessary kerosene vaults are not correctly licensed. This prevents legitimate kerosene vault additions, essential for protocol operations.
Asset Price Calculation:
The BoundedKerosineVault
is not properly configured to reference the UnboundedKerosineVault
, causing its assetPrice()
function, which depends on the unbounded vault, to fail due to an unset reference.
Collateralization and Token Minting:
Due to the flawed' KerosineManager' setup, the getKeroseneValue
function erroneously includes values from non-kerosene vaults. This impacts the calculation of collateralization ratios and the proper minting of tokens, this would lead to users being able to mint DYAD based on wrong collateral ratio assumptions.
These issues compromise the protocolβs functionality and financial integrity and could lead to significant fund loss for the protocol and users alike.
To dive deeper, first we need to understand how the KerosineManager
works:
Inside KerosineManager
, we have:
uint public constant MAX_VAULTS = 10; EnumerableSet.AddressSet private vaults;
And
function getVaults() external view returns (address[] memory) { return vaults.values(); } function isLicensed(address vault) external view returns (bool) { return vaults.contains(vault); }
Where vaults
is the AddressSet of all the Kerosene assets (bounded and non-bounded), this is evident from the following two functions in VaultManagerV2.sol
:
addKerosine()
: Link for referencegetKeroseneValue
: Link for referenceBoth of these methods make an external call to KeroseneManager
to check whether the kerosene vault is licensed via:
keroseneManager.isLicensed(address(vault)
Given this context and no additional documentation/natspec for KeroseneManager stating otherwise, we can be sure that vaults
contain kerosene vaults.
Now we have the following issues:
The deployment script incorrectly adds ethVault
and wstEth
to KerosineManager
, which should only contain kerosene vaults. This can be seen in the script:
kerosineManager.add(address(ethVault)); kerosineManager.add(address(wstEth));
The correct kerosene vaults, UnboundedKerosineVault
and BoundedKerosineVault
, are not added to KerosineManager
, leading to failed licensing checks in the addKerosene
function:
function addKerosene(uint id, address vault) external isDNftOwner(id) { if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed(); }
BoundedKerosineVault
does not have the UnboundedKerosineVault
set, which is critical for its assetPrice()
calculation:
function assetPrice() public view override returns (uint) { return unboundedKerosineVault.assetPrice() * 2; }
getKeroseneValue
will also give wrong values, as the vaults added to keroseneManager
are ethVault
and wstEth
and boundedKeroseneVault
and unboundedKeroseneVault
are left out.
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))) { usdValue = vault.getUsdValue(id); } totalUsdValue += usdValue; } return totalUsdValue; }
GitHub Links:
Even though I have provided sufficient proof via codebase implementation to assert why vaults
in KerseneManager
should contain Kerosene vaults.
But let's assume that sponsors simply made a typo. Instead of checking keroseneManager.isLicensed(vault)
, they wanted to check vaultLicenser.isLicensed(vault)
or maybe no check at all (which in itself will be a critical vulnerability).
In such a case, we would have the following scenario:
KeroseneManager
will contain a list of exogenous vaults, thus its assetPrice()
will work because it will call the following method of Vault.sol
, and it should work finefunction assetPrice() public view returns (uint) { (, int256 answer, , uint256 updatedAt, ) = oracle.latestRoundData(); if (block.timestamp > updatedAt + STALE_DATA_TIMEOUT) revert StaleData(); return answer.toUint256(); }
getKerosene
method will fail, because we won't have any kerosene vaults in the keroseneManager
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))) { <<< @audit can't have any kerosene vault in keroseneManager usdValue = vault.getUsdValue(id); } totalUsdValue += usdValue; } return totalUsdValue; }
getTotalUsdValue
and collatRatio
will also fail. If collatRatio
fails, then withdraw
will fail, and users' deposits will forever be stuck!The misconfiguration leads to a denial of service (DoS) for functionalities that rely on adding kerosene vaults or calculating asset prices in BoundedKerosineVault
. This could severely impact operational capabilities, financial calculations, and the overall reliability of the system.
Correct the KerosineManager
Configuration:
KerosineManager
. Remove the lines where non-kerosene vaults are added and ensure the correct kerosene vaults are included:
kerosineManager.add(address(unboundedKerosineVault)); kerosineManager.add(address(boundedKerosineVault));
Configure BoundedKerosineVault
Properly:
UnboundedKerosineVault
for the BoundedKerosineVault
immediately after both are instantiated:
boundedKerosineVault.setUnboundedKerosineVault(unboundedKerosineVault);
Other
#0 - c4-pre-sort
2024-04-28T18:49:18Z
JustDravee marked the issue as duplicate of #966
#1 - c4-pre-sort
2024-04-29T08:37:27Z
JustDravee marked the issue as sufficient quality report
#2 - c4-judge
2024-05-04T09:46:27Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#3 - c4-judge
2024-05-29T11:19:41Z
koolexcrypto marked the issue as duplicate of #1133
#4 - c4-judge
2024-05-29T14:03:21Z
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/main/src/core/VaultManagerV2.sol#L134-L153 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol#L50-L69 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/KerosineManager.sol#L37-L51
According to the project Readme, the kerosene tokens deposited in Unbounded Kerosene Vaults should be withdrawable.
All the withdraws are always supposed to go via the VaultManagerV2
using its withdraw()
method, which is as follows:
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); >>> //@audit 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); >>> //@audit : this is also bricked as collatRatio calls getTotalUsdValue which // calls getKeroseneValue and which finally calls assetPrice() if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); }
However, there is a critical bug in the implementation of assetPrice()
in Vault.kerosine.unbounded.sol
which prevents this.
To dive deeper, first we need to understand how the KerosineManager
works:
Inside KerosineManager
, we have:
uint public constant MAX_VAULTS = 10; EnumerableSet.AddressSet private vaults;
And
function getVaults() external view returns (address[] memory) { return vaults.values(); } function isLicensed(address vault) external view returns (bool) { return vaults.contains(vault); }
Where vaults
is the AddressSet of all the Kerosene assets (bounded and non-bounded), this is evident from the following two functions in VaultManagerV2.sol
:
addKerosine()
: Link for referencegetKeroseneValue
: Link for referenceBoth of these methods make an external call to KeroseneManager
to check whether the kerosene vault is licensed via:
keroseneManager.isLicensed(address(vault)
Given this context and no additional documentation/natspec for KeroseneManager stating otherwise, we can be sure that vaults
contain kerosene vaults.
Now, the issue arises in the implementation of assetPrice()
inside Vault.kerosine.unbounded.sol
which is defined as:
function assetPrice() public view override returns (uint) { uint tvl; /** * @audit -> vaults will be KerosineVaults (both bounded and unbounded) and not * the exogenous valuts */ >>> 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() * // this will lead to recursive call 1e18) / (10 ** vault.asset().decimals()) / (10 ** vault.oracle().decimals()); // @audit -> no oracle defined in bounded and unbounded vaults } uint numerator = tvl - dyad.totalSupply(); uint denominator = kerosineDenominator.denominator(); return (numerator * 1e8) / denominator; }
Since vaults
will be a list of Kerosene Vaults, hence vault.assetPrice()
inside the loop will lead to calling UnboundedVault's assetPrice()
which is a recursive call.
Inside BoundedVault too, the assetPrice()
is defined as return unboundedKerosineVault.assetPrice() * 2;
so this will also in turn call assetPrice
of unbounded kerosene vault.
Moreover there is no oracle
defined in Bounded and Unbounded vaults as the prices were supposed to be calculated deterministically.
Hence UnboundedKerosineVault::assetPrice()
will always revert.
Thus a call to VaultManagerV2::withdraw(uint id, address vault, uint amount, address to)
will always fail when the concerned vault is UnboundedKeroseneVault
.
Given that assetPrice()
of UnboundedKerosineVault
is faulty, this will also affect withdrawals of exogenous vaults!
As soon as a person adds Unbounded/Bounded Kerosene vault via addKerosene
, it will affect withdrawals for all the other vaults including exogenous vaults. The reason being, in withdrawal()
we have the following line
if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
collatRatio and the subsequent called functions are defined as:
function collatRatio(uint id) public view returns (uint) { uint _dyad = dyad.mintedDyad(address(this), id); if (_dyad == 0) return type(uint).max; return getTotalUsdValue(id).divWadDown(_dyad); } function getTotalUsdValue(uint id) public view returns (uint) { return getNonKeroseneValue(id) + getKeroseneValue(id); } function getKeroseneValue(uint id) public view returns (uint) { ... code block if (keroseneManager.isLicensed(address(vault))) { usdValue = vault.getUsdValue(id); } ... }
And inside Vault.kerosene.sol we have:
function getUsdValue(uint id) public view returns (uint) { return (id2asset[id] * assetPrice()) / 1e8; }
Paste the following test in VaultManager.t.sol
<Details> <Summary> Test of assetPrice </Summary>imports in VaultManager.t.sol
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol"; import {BoundedKerosineVault} from "../src/core/Vault.kerosine.bounded.sol"; import {ERC20Mock} from "./ERC20Mock.sol"; import {KerosineManager} from "../src/core/KerosineManager.sol"; import {ERC20} from "@solmate/src/tokens/ERC20.sol";
Test at the bottom
ERC20Mock public keroseneToken; KerosineManager public keroseneManager; UnboundedKerosineVault public keroseneVaultUnbounded; BoundedKerosineVault public keroseneVaultBounded; address user1 = makeAddr("user1"); address user2 = makeAddr("user2"); uint dnftId1; uint dnftId2; uint amount = 1e18; function setupKeroseneVaults() public { keroseneToken = new ERC20Mock("Kerosene", "KERO"); keroseneManager = new KerosineManager(); keroseneVaultBounded = new BoundedKerosineVault( vaultManager, ERC20(address(keroseneToken)), keroseneManager ); keroseneVaultUnbounded = new UnboundedKerosineVault( vaultManager, ERC20(address(keroseneToken)), dyad, keroseneManager ); vm.deal(user1, 100 ether); vm.deal(user2, 100 ether); keroseneToken.mint(address(user1), 10 * amount); keroseneToken.mint(address(user2), 10 * amount); // set kerosenemanager to vaultmanager vaultManagerV2.setKeroseneManager(keroseneManager); // set bounded and unbounded vaults to kerosenemanager keroseneManager.add(address(keroseneVaultBounded)); keroseneManager.add(address(keroseneVaultUnbounded)); } function setupDnfts() public { // mint two DNfts to user1 and user2 vm.startPrank(user1); dnftId1 = dNft.mintNft{value : 1 ether}(user1); dnftId2 = dNft.mintNft{value : 1 ether}(user2); console.log("DNFT ID 1: ", dnftId1); console.log("DNFT ID 2: ", dnftId2); vm.stopPrank(); } function setupVaultManager() public { // license the vault vm.prank(vaultLicenser.owner()); // vaultLicenser.add(address(keroseneVaultUnbounded)); vaultLicenser.add(address(keroseneVaultBounded)); vm.stopPrank(); } function test_assetPriceKeroseneUnbounded() public{ setupKeroseneVaults(); setupDnfts(); setupVaultManager(); // add unbounded kerosene vault to id vm.startPrank(user1); vaultManagerV2.addKerosene(dnftId1, address(keroseneVaultUnbounded)); // check asset price console.log("asset price: ", keroseneVaultUnbounded.assetPrice()); vm.stopPrank(); }
Also add following imports to BaseTest.sol
</Details> <Details> <Summary> Console output </Summary>import {VaultManagerV2} from "../src/core/VaultManagerV2.sol"; contract { // rest of state variables VaultManagerV2 vaultManagerV2; function setUp() public{ // rest of code vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser); // rest of code }
</Details>[3074] UnboundedKerosineVault::assetPrice() [staticcall] β ββ [1342] KerosineManager::getVaults() [staticcall] β β ββ β [Return] [0x15cF58144EF33af1e14b5208015d11F9143E27b9, 0x212224D2F2d262cd093eE13240ca4873fcCBbA3C] β ββ [192] BoundedKerosineVault::oracle() [staticcall] β β ββ β [Revert] EvmError: Revert β ββ β [Revert] EvmError: Revert ββ β [Revert] EvmError: Revert
Even though I have provided sufficient proof via codebase implementation to assert why vaults
in KerseneManager
should contain Kerosene vaults.
But let's assume that sponsors simply made a typo. Instead of checking keroseneManager.isLicensed(vault)
, they wanted to check vaultLicenser.isLicensed(vault)
or maybe no check at all (which in itself will be a critical vulnerability).
In such a case, we would have the following scenario:
KeroseneManager
will contain a list of exogenous vaults, thus its assetPrice()
will work because it will call the following method of Vault.sol
, and it should work finefunction assetPrice() public view returns (uint) { (, int256 answer, , uint256 updatedAt, ) = oracle.latestRoundData(); if (block.timestamp > updatedAt + STALE_DATA_TIMEOUT) revert StaleData(); return answer.toUint256(); }
getKerosene
method will fail, because we won't have any kerosene vaults in the keroseneManager
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))) { <<< @audit can't have any kerosene vault in keroseneManager usdValue = vault.getUsdValue(id); } totalUsdValue += usdValue; } return totalUsdValue; }
getTotalUsdValue
and collatRatio
will also fail. If collatRatio
fails, then withdraw
will fail, and users' deposits will forever be stuck!As soon as Kerosene Vault is added to corresponding to dNft, all the deposits will be stuck, and any kind of withdrawal
, mintDyad
, and liquidate
will fail. As withdraw checks at the end:
if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
Manual Review, Foundry
To mitigate the issue, since vaults
in KeroseneManager can only have either kerosene or exogenous vaults.
For clarity, I would recommend that we keep track of kerosene-related vaults in KeroseneManger
and add an AddressSet
in VaultManagerV2.sol
or Vault.sol
for exogenous vaults and implement a getter to fetch all the licensed vaults.
+ EnumerableSet.AddressSet private exogenousVaults; /// @inheritdoc IVaultManager 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(); + if(!exogenousVaults.contains(vault)) exogenousVaults.add(vault); emit Added(id, vault); } + function getExogenousVaults() external returns (address[] memory){ + return exogenousVaults.values(); + }
Then change the withdraw()
inside UnboundedKeroseneVault
as:
function assetPrice() public view override returns (uint) { uint tvl; - address[] memory vaults = kerosineManager.getVaults(); + address[] memory vaults = vaultManager.getExogenousVaults(); 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; }
Context
#0 - c4-pre-sort
2024-04-26T21:49:43Z
JustDravee marked the issue as primary issue
#1 - c4-pre-sort
2024-04-26T21:49:46Z
JustDravee marked the issue as high quality report
#2 - shafu0x
2024-04-30T11:44:23Z
Valid. The main issue here is that it does not have an oracle
.
#3 - koolexcrypto
2024-05-04T10:09:51Z
Since vaults will be a list of Kerosene Vaults, hence vault.assetPrice() inside the loop will lead to calling UnboundedVault's assetPrice() which is a recursive call.
KerosineManager hasethVault
and wstEth
vaults. So, recursive call is not going to happen.
Check deployment script:
KerosineManager kerosineManager = new KerosineManager(); kerosineManager.add(address(ethVault)); kerosineManager.add(address(wstEth));
Moreover there is no oracle defined in Bounded and Unbounded vaults as the prices were supposed to be calculated deterministically. Hence UnboundedKerosineVault::assetPrice() will always revert.
Marking it as a duplicate of #830
#4 - c4-judge
2024-05-04T10:10:25Z
koolexcrypto marked the issue as duplicate of #830
#5 - c4-judge
2024-05-11T20:04:29Z
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
In liquidate
method in VaultManagerV2
, as per the documentation, if the Collateral value of a note (DNft) USD drops below 150% of its DYAD, it should face liquidation.
To encourage liquidation, the liquidator should receive a 20% bonus of the target Noteβs backing collateral. This scenario only works fine if the Collateral Ratio is between (100-150%).
However, suppose the ratio drops below 100%. In that case, a liquidator has no incentive to liquidate the position, and neither will the owner of the collateral try to liquidate it because the value of DYAD with them is more than the backing collateral.
We have the following logic for liquidation:
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); }
where
function collatRatio(uint id) public view returns (uint) { uint _dyad = dyad.mintedDyad(address(this), id); if (_dyad == 0) return type(uint).max; return getTotalUsdValue(id).divWadDown(_dyad); }
Now let's take an example in which the collateral ratio goes below 1e18; note MIN_COLLATERIZATION_RATIO = 1.5e18 and LIQUIDATION_REWARD = 0.2e18
Let's assume the following scenario:
stETH
(or 1e18 in terms of decimals) in the stETH
vault; let's take the price of stETH
is USD 3000 corresponding to id = 1
DYAD
token or 1000e18 DYAD, therefore cr
= (totalUSDValue) * 1e18 / (mintedDyad) = 1.5e18
.cr
= (1500)/(2000)e18 = 0.75e18
.cr
= 0.75e18DYAD
as it burns the entire DYAD
corresponding to id, and mintedDyad
of id
will become 0.cappedCr
= 1e18liquidationEquityShare
= (1e18-1e18).mulWadDown(LIQUIDATION_REWARD)
= 0liquidationAssetShare
= (0 + 1e18).divWadDown(cappedCr)
= 1e18vault.id2asset(id).mulWadUp(liquidationAssetShare)
i.e 1e18 or 1 stETHHence, nobody will try to liquidate this position
This would leave the protocol at a major loss, and the owner of id
can simply choose not to redeem DYAD as they have more worth of DYAD than the collateral deposited.
Severity: High, lots of potential loss could be avoided Likelyhood: Low, depends on major swing in collateral asset Conclusion: Medium severity
Manual review
This can easily be mitigated by changing the liquidate method so that it does not always burn the mintedDyad corresponding to ID and proportionately burns only the amount necessary so that the liquidator gets rewarded and all of the backing collateral is allocated to the liquidator with profit.
function liquidate(uint id, uint to) external isValidDNft(id) isValidDNft(to) { // collateral value = 1500, mintedDyad(id) = 2000 uint cr = collatRatio(id); // 0.75e18 if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh(); - dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id)); + if (cr >= 1e18){ + dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id)); + }else{ + unit amountToBurn = getTotalUsdValue(id) * 0.95 // amountToBurn -> 1425 offering 5% discount (can be changed) + dyad.burn(id, msg.sender, amountToBurn); // mintedDyad(id) = 2000-1425 = 575 + } uint cappedCr = cr < 1e18 ? 1e18 : cr; // will be 1e18 uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown( LIQUIDATION_REWARD ); // will be 0 uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr ); // will be 1e18 uint numberOfVaults = vaults[id].length(); // will be 1 as we are taking only 1 vault for now for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[id].at(i)); uint collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare); // will be 1 stETH vault.move(id, to, collateral); /** * id2Asset[id]-= 1e18 = 0 * id2Asset[to]+= 1e18 */ } emit Liquidate(id, msg.sender, to); }
In this scenario, the protocol got back 1425 out of 2000 mintedDyad, and the liquidators had an incentive to liquidate the position by burning 1425 DYAD but receiving 1500 worth of shares (1 stETH) allocated to them.
Other
#0 - c4-pre-sort
2024-04-28T11:09:31Z
JustDravee marked the issue as duplicate of #977
#1 - c4-pre-sort
2024-04-29T09:24:02Z
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:57Z
koolexcrypto marked the issue as unsatisfactory: Invalid
#4 - c4-judge
2024-05-12T09:46:32Z
koolexcrypto marked the issue as grade-c
#5 - c4-judge
2024-05-28T16:20:18Z
This previously downgraded issue has been upgraded by koolexcrypto
#6 - c4-judge
2024-05-28T16:21:42Z
koolexcrypto marked the issue as satisfactory