DYAD - 0xnilay's results

The first capital efficient overcollateralized stablecoin.

General Information

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

DYAD

Findings Distribution

Researcher Performance

Rank: 92/183

Findings: 3

Award: $21.39

🌟 Selected for report: 0

πŸš€ Solo Findings: 0

Lines of code

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

Vulnerability details

Description

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:

  1. 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.

  2. 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.

  3. 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.

Proof of Concept

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:

  1. addKerosine() : Link for reference
  • This method links a Kerosene Vault to a particular dNft.
  1. getKeroseneValue : Link for reference
  • This method returns the total USD value of Kerosene inside Bounded/unbounded vaults.

Both 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.

    • This function is supposed to return the total value of all Kerosene-related assets, not the exogenous ones.
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:

Alternate Assumption:

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:

  1. 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 fine
function assetPrice() public view returns (uint) {
    (, int256 answer, , uint256 updatedAt, ) = oracle.latestRoundData();
    if (block.timestamp > updatedAt + STALE_DATA_TIMEOUT)
        revert StaleData();
    return answer.toUint256();
}
  1. However, now the 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;
}
  1. This would mean that getTotalUsdValue and collatRatio will also fail. If collatRatio fails, then withdraw will fail, and users' deposits will forever be stuck!

Impact

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.

Tools Used

  • Solidity: The smart contracts are written in Solidity.
  • Forge: Assumed use for scripting deployments.
  • Manual Review: Analyzing the deployment script and related smart contracts.
  1. Correct the KerosineManager Configuration:

    • Modify the deployment script to ensure that only kerosene-related vaults are managed by 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));
  2. Configure BoundedKerosineVault Properly:

    • Update the deployment script to set the UnboundedKerosineVault for the BoundedKerosineVault immediately after both are instantiated:
      boundedKerosineVault.setUnboundedKerosineVault(unboundedKerosineVault);

Assessed type

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

Awards

3.8221 USDC - $3.82

Labels

bug
3 (High Risk)
high quality report
satisfactory
sponsor confirmed
edited-by-warden
:robot:_28_group
duplicate-830

External Links

Lines of code

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

Vulnerability details

Description

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.

Proof of Concept

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:

  1. addKerosine() : Link for reference
  • This method links a Kerosene Vault to a particular dNft.
  1. getKeroseneValue : Link for reference
  • This method returns the total USD value of Kerosene inside Bounded/unbounded vaults.

Both 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.

Additional impact on other vaults!

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; 
    }

Coded POC

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

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> <Details> <Summary> Console output </Summary>
[3074] UnboundedKerosineVault::assetPrice() [staticcall]
    β”‚   β”œβ”€ [1342] KerosineManager::getVaults() [staticcall]
    β”‚   β”‚   └─ ← [Return] [0x15cF58144EF33af1e14b5208015d11F9143E27b9, 0x212224D2F2d262cd093eE13240ca4873fcCBbA3C]
    β”‚   β”œβ”€ [192] BoundedKerosineVault::oracle() [staticcall]
    β”‚   β”‚   └─ ← [Revert] EvmError: Revert
    β”‚   └─ ← [Revert] EvmError: Revert
    └─ ← [Revert] EvmError: Revert
</Details>

Alternate Assumption:

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:

  1. 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 fine
function assetPrice() public view returns (uint) {
    (, int256 answer, , uint256 updatedAt, ) = oracle.latestRoundData();
    if (block.timestamp > updatedAt + STALE_DATA_TIMEOUT)
        revert StaleData();
    return answer.toUint256();
}
  1. However, now the 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;
}
  1. This would mean that getTotalUsdValue and collatRatio will also fail. If collatRatio fails, then withdraw will fail, and users' deposits will forever be stuck!

Impact

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();

Tools Used

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;
}

Assessed type

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

Awards

17.2908 USDC - $17.29

Labels

bug
2 (Med Risk)
satisfactory
sufficient quality report
:robot:_11_group
duplicate-977

External Links

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L205-L228

Vulnerability details

Description

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.

Proof of Concept

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:

  1. User deposited 1 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
  2. User minted 2k USD worth of DYAD token or 1000e18 DYAD, therefore cr = (totalUSDValue) * 1e18 / (mintedDyad) = 1.5e18.
  3. Due to a market crash, the value of ETH went to 1500 USD, now cr = (1500)/(2000)e18 = 0.75e18.
  4. According to the liquidate method:
    • cr = 0.75e18
    • caller will burn 2k worth of DYAD as it burns the entire DYAD corresponding to id, and mintedDyad of id will become 0.
    • therefore cappedCr = 1e18
    • liquidationEquityShare = (1e18-1e18).mulWadDown(LIQUIDATION_REWARD) = 0
    • liquidationAssetShare = (0 + 1e18).divWadDown(cappedCr) = 1e18
    • assset allocated to caller (to) will be vault.id2asset(id).mulWadUp(liquidationAssetShare) i.e 1e18 or 1 stETH
  5. So effectively, the caller burnt 2k worth of tokens and only received 1500 worth of assets, i.e., a 500 worth of loss.

Hence, nobody will try to liquidate this position

Impact

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

Tools Used

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.

Hence, the protocol's loss will be minimised by a massive 71%, from 2000 DYAD to 575 DYAD!!

Assessed type

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

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax Β© 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter