DYAD - oakcobalt's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L56

Vulnerability details

Impact

TVL might be calculated based on incorrect vaults, which will cause incorrect price or tx reverts.

Proof of Concept

In Vault.kerosine.unbounded.sol - assetPrice(), kerosine price needs to be calculated based on TVL of non-kerosine collateral values (Weth, Wsteth,etc.). Based on doc, TVL should exclude kerosine.

C: the total USD value of all exogenous collateral in the protocol (TVL). Critically, this total does not include Kerosene itself

However, kerosine asset vaults might be used instead of only non-keorsine vaults in current implementation, because assetPrice() calls kerosineManager.getVaults(). And kerosineManger might include kerosine assets, according to VaultManagerV2.

//src/core/Vault.kerosine.unbounded.sol
  function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      uint tvl;
|>      address[] memory vaults = kerosineManager.getVaults(); //@audit kerosineManager might include kerosine vaults, based on imple. in VaultManagerV2::addKerosene
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L56)

Based on VaultManagerV2.sol, kerosineManager is used to license and store addresses of kerosine vaults. For example, in VaultMangerV2::addkerosene (method to add kerosine vaults to user Note id), keroseneManager.isLicensed(vault) is called.

//src/core/VaultManagerV2.sol
  function addKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
...
    //@audit-info note: kerosene vault is intended to be added to keroseneManager
    if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L88C1-L88C87)

If keroseneManger contains kerosene vaults' addresses, then calling kerosineManager.getVaults() in Vault.kerosine.unbounded::assetPrice will result in incorrect vault assets(kerosine assets) to be added in TVL. kerosine price will be incorrect, which affects all calls that consume kerosine price/value such as VaultManagerV2::collaRatio, VaultManagerV2::withdraw, VaultManagerV2::mintDyad, VaultManagerV2::liquidate. ( collatRatio -> getTotalUsdValue -> getKeroseneValue -> vault.getUsdValue -> vault.assetPrice )

In addition, if kerosineManager.getVaults() also includes the calling Vault.kerosine.unbounded contract address(self), assetPrice() will result in a recursive call.

Tools Used

Manual

Recommended Mitigation Steps

Get strictly non-kerosene vaults in Vault.kerosine.unbounded::assetPrice.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L75

Vulnerability details

Impact

Users can add kerosene assets to be used as non-kerosene collaterals due to insufficient check in VaultManagerV2.

Proof of Concept

Dyad can only be minted against non-kerosene collaterals and based on doc, kerosene represents surplus of collateral and cannot be used as additional colalterals.

Kerosene is thus as valuable as the degree of DYAD’s overcollateralization. Kerosene is not additional collateral;...

However, In VaultManagerV2.sol, the checks in add() is insufficient, which allows users to add kerosene vault to their dnft id , and use kerosene assets as non-kerosene collaterals. They can mint dyad directly against kerosene.

//src/core/VaultManagerV2.sol
  function add(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {

    if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
    //@audit this check is insufficient. Because vaultLicenser also contains kerosene vault(Vault.kerosene.unbounded.sol), a user can directly add kerosene vault to vaults[id], which will be valued as non-kerosene collaterals.
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
    if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L75)

For reference, according to Deploy.V2.s.sol, both non-kerosene vaults (weth vault and wsteth vault) and kerosene vault (Vault.kerosine.unbounded) are added to vaultLicenser. The sponsor also confirmed this intention in discord video.

If a user adds vault.kerosine.unbounded to vaults[id] through add(). In getNonKeroseneValue(), their kerosine assets will be valued as non-kerosine assets. This further allows a user to mint dyad against kerosene asset directly, putting dyad pegging at risk.

Tools Used

Manaul

Recommended Mitigation Steps

In VaultMangerV2::add, add additional checks to ensure a user can only add non-kerosene vault to vaults[id].

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L150

Vulnerability details

Impact

Users who only have kerosine collaterals will have their tokens locked in the vault, due to vulnerable underflow condition.

Proof of Concept

In VaultManagerV2.sol, a user can deposit both kerosine collaterals and non-kerosine(weth, wsteth,etc) collaterals. Based on doc, the kerosine token can be acquired through staking or secondary market trading.

The secondary market may trade Kerosene above its deterministic protocol-defined value...

A user might only have kerosine collaterals in VaultMangerV2.sol if (1) the user acquired kerosine from a secondary market and only deposited kerosine; (2) Or the user withdrew all non-kerosine collaterals.

In such case, the user's kerosine deposit will be locked due to a vulnerable underflow condition in withdraw().

Regardless of dyad minting (dyad.mintedDyad(address(this), id)), withdraw() will always subtract the pending withdrawal asset value (kerosine value in this case) from total non-kerosine collateral value (getNonKeroseneValue(id)).

//src/core/VaultManagerV2.sol
  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
...
    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();
    //@audit edge case: user only has kerosine deposited. This check will revert due to underflow: 0 - value 
|>  if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L150) As seen above, in this case 0 - value (user depoisted kerosene value) will underflow and revert the tx. The user would have to pay gas and deposit a greater value of non-kerosene collateral tokens.

Tools Used

Manual

Recommended Mitigation Steps

When a user is withdrawing a kerosene asset, there is no need to subtract kerosene asset value from non-kerosene collateral value, because dyad currently can only be minted against non-kerosene collaterals. In such cases, only collateral ratio check after withdraw should suffice.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L150

Vulnerability details

Impact

User deposited kerosine can be locked even with no dyad minting.

Proof of Concept

In VaultManagerV2.sol, withdraw() has a vulnerable check on collateral values which might result in underflow revert in normal conditions when a user has no dyad minted.

In withdraw(), a check will subtract the pending withdrawal asset value from non-keroseneValue (getNonKeroseneValue(id)). if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

A vulnerable case is whenever a user has more kerosene collateral than non-kerosene collaterals, trying to withdraw the kerosene collateral will revert due to an underflow error (getNonKeroseneValue(id) < value). The user might have no dyad minted.

Suppose a user deposited 110 usd kerosine, 100 usd Weth and minted 0 dyad (max collateral ratio). Withdrawing 110 usd kerosine will revert, due to underflow.

//src/core/VaultManagerV2.sol
  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
...
    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L150)

Tools Used

Manual

Recommended Mitigation Steps

Since a user cannot mint dyad against kerosene collateral in current VaultManagerV2.sol implementation, when withdrawing asset is kerosene, no need to subtract the kerosene value from the non-kerosene value.

Assessed type

Other

Judge has assessed an item in Issue #758 as 2 risk. The relevant finding follows:

Low -05 In normal conditions, well-collateralized users might not be able to withdraw kerosine deposits Instances(1) User who deposited both kerosine and non-kerosine collaterals, minted dyad and well over collateralized might not be able to withdraw kerosine.

In VaultMangerV2::withdraw, there is a check if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

However, it’s problematic to directly subtract withdrawal value from getNonKeroseneValue(id) without checking whether the withdrawal asset is kerosine or non-kerosene.

If the user is withdrawing kerosine asset then, whenever a user has greater kerosine collaterals than non-kerosene collaterals, this will trigger reverts due to underflow even though the user can be well over-collateralized.

Recommendations: Add a check to see whether vault is a kerosine vault. Only subtract value from getNonKeroseneValue(id) when checking first that value is non-kerosene assets.

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L65

Vulnerability details

Impact

VaultManagerV2 is vulnerable to DOS due to vulnerable underflow risk in Vault.kerosine.unbounded::assetPrice.

Proof of Concept

In Vault.kerosine.unbounded::assetPrice, the numerator is calculated based on tvl - dyad.totalSupply(). This is vulnerable to underflow revert when collateral asset price fluctuates which results in tvl value drops.

//src/core/Vault.kerosine.unbounded.sol
  function assetPrice() 
    public 
    view 
    override
    returns (uint) {
...
|>    uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L65)

This is more likely to happen at the early phase of VaultMangerV2 when there are only a few depositors who maximally minted dyad against their collaterals. And vault assets lack diversity (e.g. only weth is deposited) and are subject to high price volatility.

Suppose at T1, tvl(non-kerosine) = 1000 usd, kerosene collateral = 500 usd dyat.totalSupply = 900 usd.

Then at T2, weth price drops sharply. tvl(non-kerosine) = 850 usd, dyad.totalSupply = 900 usd. Now tvl < dyad.totalsupply.

This will cause Vault.kerosine.unbounded::assetPrice to revert due to underflow error, which DOS critical VaultManagerV2 flows that checks collateral ratio(collatRatio(id)) and consumes kerosine price value, including the liquidation flow.

Tools Used

Manual

Recommended Mitigation Steps

Since kerosine token price cannot be negative, consider checking if tvl < dyad.toatalSupply(), return 0 value, indicating no surplus, and allows liquidation when the protocol needs it the most.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L88

Vulnerability details

Impact

Users cannot add kerosene vaults to vaultsKerosene due to an incorrect check, a user's kerosene collateral value will be incorrect

Proof of Concept

In VaultManagerV2.sol, a user is supposed to add kerosene vaults to their account(dNft id) through addKerosene().

However, addKerosene() will revert when a user trying to add a kerosene vault due to incorrect check if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed().

//src/core/VaultManagerV2.sol
  function addKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
    //@audit this is incorrect check. keroseneManager will only hold non-kerosene vaults (weth vault,wsteth vault). adding a kerosene vault will be reverted.
|>  if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L88)

According to discord, the intention for keroseneManager is to only hold exogeneous asset vaults (weth,wsteth, etc) to calculate deterministic kerosene asset price. This is also echoed in Deploy.V2.s.sol where only weth vault and wsteth vault are added to keroseneManager.

//script/deploy/Deploy.V2.s.sol
    function run() public returns (Contracts memory) {
...
        kerosineManager.add(address(ethVault));
        kerosineManager.add(address(wstEth));
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65)

Back to addKerosene(), when a user is adding a kerosene vault(e.g. Vault.kerosene.unbounded.sol), !keroseneManager.isLicensed(vault) will evaluate to true, due to keroseneManger is not intended to hold any kerosene vaults, which will directly trigger revert.

A user can never add a kerosene vault, and if they try to add a non-kerosene vault(e.g. weth vault) through addKerosene(), the tx will succeed. This will cause exogenous assets to be valued as kerosene value in getKeroseneValue(). A user's kerosene collateral value will either be zero or incorrect.

Tools Used

Manual

Recommended Mitigation Steps

In addKerosene(), change the check to if (!vaultLicenser.isLicensed(vault)) revert VaultNotLicensed();Also, since vaultLicenser is intended to license both kerosene and non-kerosene vault, consider adding additional check to ensure only kerosene vaults are added to vaultsKerosene[id]`.

Assessed type

Other

Low -01 Donation attacks can manipulate kerosine price.

Instances(1) The current implementation of kerosine price is dependent on a denominator value which is kerosine.totalSupply() - kerosine.balanceOf(MAINNET_OWNER), this is the kerosine that is actually circulating (e.g. earned from staking rewards).

Based on Vault.kerosine.unbounded::assetPrice, kerosine price = (tvl- dyad.totalSupply()) / (kerosine.totalSupply() - kerosine.balanceOf(MAINNET_OWNER))

However, using kerosine.balanceOf(MAINNET_OWNER) to compute the denominator is vulnerable, because balanceOf can be manipulated by any user donating kerosine to MAINNET_OWNER.

//src/staking/KerosineDenominator.sol
  function denominator() external view returns (uint) {
|>    return kerosine.totalSupply() - kerosine.balanceOf(MAINNET_OWNER);
  } 

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/staking/KerosineDenominator.sol#L21)

Such donation attack might be profitable in some circumstances such as arbitrage. Based on doc, kerosine can be traded on a secondary market, and arbitrage is allowed.

The secondary market may trade Kerosene above its deterministic protocol-defined value. ...

Donating kerosine to MAINNET_OWNER when kerosine price is low, might be relatively low cost which encourages arbitrage through price manipulation.

Recommendations: Consider using a different state variable in place of kerosine.balanceOf(MAINNET_OWNER)).

Low -02 Missing deployment step - setUnboundedKerosineVault needs to be called by the owner during migration.

Instances(1) In Deploy.V2.s.sol, all related contract deployment and initialization steps are performed. According to readme,

The only transaction that needs to be done by the multi-sig after the deployment is licensing the new Vault Manager.

However, this is not true because Deploy.V2.s.sol missed setting UnbouldedKerosineVault contract address in Vault.kerosine.bounded.sol.

//src/core/Vault.kerosine.bounded.sol
    function setUnboundedKerosineVault(
        UnboundedKerosineVault _unboundedKerosineVault
    ) external onlyOwner {
        unboundedKerosineVault = _unboundedKerosineVault;
    }

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.bounded.sol#L29)

As a result, the multi-sig needs to call setUnboundedKerosineVault after the migration to enable bounded kerosine vault.

Recommendations: In Deploy.V2.s.sol::run, add boundedKerosineVault.setUnboundedKerosineVault(address(unboundedKerosineVault))

Low -03 Insufficient checks in VaultManagerV2 might allow users to deposit/withdraw from any unlicensed / potentially malicious vault, user might lose funds.

Instances(3) In VaultManagerV2::deposit / VaultManagerV2::withdraw /VaultManagerV2::redeemDyad , no checks on input vault arguments. As a result, any _vault can be used.

In case of unlicensed vault is used for deposit, users deposit will not be accounted as collaterals.

In case of malicious vault address is used for deposit, the user might lose funds. (1)

//src/core/VaultManagerV2.sol
    function deposit(
        uint id,
        address vault,
        uint amount
    ) external isValidDNft(id) {
    idToBlockOfLastDeposit[id] = block.number;
 |>   Vault _vault = Vault(vault); //@audit no check on vault is added /licensed
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L128) (2)

//src/core/VaultManagerV2.sol
  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 no check on vault is added /licensed
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L145) (3)

//src/core/VaultManagerV2.sol
  function redeemDyad(
    uint    id,
    address vault,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
    returns (uint) { 
      dyad.burn(id, msg.sender, amount);
|>    Vault _vault = Vault(vault); //@audit no check on vault is added /licensed
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L194) Recommendations: In deposit / withdraw / redeemDyad, check input vault is added in NOTE id and is licensed.

Low -04 The use of balanceOf(address(vault)) makes assetPrice() vulnerable to donation attack.

Instances(1) In Vault.kerosine.unbounded.sol - assetPrice(), when calcualting tvl, balanceOf(address(vault)) is used to calculate non-kerosene assets value in deposited.

However, balanceOf(address(vault)) may not reflect actual deposits and is vulnerable to donation attacks. Especially when in the early phases when total deposit of asset is very low, an attack can perform low-cost donation attack to manipulate kerosine price. And such donation attack may profit the attacker in the context of arbitrage with a secondary market.

Based on doc, arbitrage is a possible scenario:

The secondary market may trade Kerosene above its deterministic protocol-defined value....

Recommendations: Consider avoid using balanceOf(address(vault).

Low -05 In normal conditions, well-collateralized users might not be able to withdraw kerosine deposits

Instances(1) User who deposited both kerosine and non-kerosine collaterals, minted dyad and well over collateralized might not be able to withdraw kerosine.

In VaultMangerV2::withdraw, there is a check if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

However, it's problematic to directly subtract withdrawal value from getNonKeroseneValue(id) without checking whether the withdrawal asset is kerosine or non-kerosene.

If the user is withdrawing kerosine asset then, whenever a user has greater kerosine collaterals than non-kerosene collaterals, this will trigger reverts due to underflow even though the user can be well over-collateralized.

Recommendations: Add a check to see whether vault is a kerosine vault. Only subtract value from getNonKeroseneValue(id) when checking first that value is non-kerosene assets.

Low -06 Consider unify spelling of kerosene(kerosine) in code

Instance(7) In most cases, kerosine is used in the code in scope, with a few exceptions. (1)

   KerosineManager public keroseneManager;

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L32) (2)

    mapping(uint => EnumerableSet.AddressSet) internal vaultsKerosene;

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L35) (3)

    function setKeroseneManager(
        KerosineManager _keroseneManager
    ) external initializer {
        keroseneManager = _keroseneManager;
    }

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L59) (4)

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

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L80) (5)

    function removeKerosene(uint id, address vault) external isDNftOwner(id) {
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L106) (6)

    function getNonKeroseneValue(uint id) public view returns (uint) {

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L250) (7)

    function getKeroseneValue(uint id) public view returns (uint) {

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L269)

Recommendations: Unify spelling.

Low -07 Unnecessary math in some conditions

Instance(1) In VaultManagerV2.sol - liquidate(), it's not aways necessary to calculate liquidationAssetShare value.

When uint cappedCr == 1e18, liquidationAssetShare will aways be 1e18, which indicates 100% collateral of liquidatee will be moved to the liquidator.

//src/core/VaultManagerV2.sol
    function liquidate(
        uint id,
        uint to
    ) external isValidDNft(id) isValidDNft(to) {
    ...
            uint cappedCr = cr < 1e18 ? 1e18 : cr;
                    uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(
            LIQUIDATION_REWARD
        );
|>                uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(
            cappedCr
        );
...

(https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L219) Recommendations: Add a control flow when cappedCr ==1e18, no need to recalculate liquidationAssetShare.

Low -08 Unused modifier

Instances(1) IN VaultMangerV2.sol, this modifier isLicensed is not used.

    modifier isLicensed(address vault) {
        if (!vaultLicenser.isLicensed(vault)) revert NotLicensed();
        _;
    }

Recommendations: remove unused modifier.