🔴 H01 - Malicious user can block withdrawals for another user by depositing in dummy malicious vault
To prevent users from exploiting oracle updates for arbitrage, a safeguard has been implemented. This safeguard makes it impossible to deposit and then withdraw in the same block number.
But there 3 issues how this is implemented in VaultManagerV2::deposit:
- No check that vault is licensed
- Combined with the fact that user1 can deposit in user2 dNFT
- Depositing in vault1 for a dNFT block withdrawals for all vault of this dNFT
This make it possible for a malicious user to call deposit with a empty dummy vault, which will lock withdrawal for the victim for the present block for all vaults
Even with no dummy vault, a malicious user could deposit dust amount to any of the victim vaults to prevent him from withdrawing in same block
Possibility to prevent user from withdrawing
File: src/core/VaultManagerV2.sol
119: function deposit(
120: uint id,
121: address vault,
122: uint amount
123: )
124: external
125:❌ isValidDNft(id) //<(1): missing isLicensed(vault) modifier
126: {
127:❌ idToBlockOfLastDeposit[id] = block.number; //<(3): locking withdrawal for all vaults for this id
128: Vault _vault = Vault(vault);
129: _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
130: _vault.deposit(id, amount);
131: }
132:
133: /// @inheritdoc IVaultManager
134: function withdraw(
135: uint id,
136: address vault,
137: uint amount,
138: address to
139: )
140: public
141: isDNftOwner(id)
142: {
143:❌ if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock(); //<(3)
144: uint dyadMinted = dyad.mintedDyad(address(this), id);
145: Vault _vault = Vault(vault);
146: uint value = amount * _vault.assetPrice()
147: * 1e18
148: / 10**_vault.oracle().decimals()
149: / 10**_vault.asset().decimals();
150: if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
151: _vault.withdraw(id, to, amount);
152: if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
153: }manual review
- Verify vault is licensed (could also use the unused
hasVaultwhich might be even better) - Depositing on behalf of someone else should be permissioned, for example by implementing a permission mechanism:
mapping(address owner => address) allowed+ modifierisAllowed(address))
: src/core/VaultManagerV2.sol:119
+ mapping(address owner => address) allowed;
function deposit(
uint id,
address vault,
uint amount
)
external
isValidDNft(id)
+ isLicensed(vault)
+ isAllowed(msg.sender)
{
idToBlockOfLastDeposit[id] = block.number;
Vault _vault = Vault(vault);
_vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
_vault.deposit(id, amount);
}🔴 H02 - Based on deployment script, eth/wsteth vault are in both Licenser and keroseneManager, causing double colleteral counting
https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/VaultManagerV2.sol#L250-L286 https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/script/deploy/Deploy.V2.s.sol#L95
DYAD protocol allow users to deposit collateral and mint a dollar-pegged asset (DYAD) against it. Two types of vaults are proposed:
- exogenous vaults, where users can deposit assets external to the protocol (like WETH, wstETH, ...)
- kerosine vaults, where user can deposit the endogenous token from the protocol, Kerosine
The issue is that right now, the deploy script declare WETH and wstETH vaults as both being exogenous and kerosine vaults. Also, unboundedKerosineVault is licensed in Licenser while it should be in KerosineManager
This completely mess-up the collatral accounting.
File: script/deploy/Deploy.V2.s.sol
61:
62: KerosineManager kerosineManager = new KerosineManager();
63:
64:❌ kerosineManager.add(address(ethVault)); //<@audit: should not be added in KerosineManager
65:❌ kerosineManager.add(address(wstEth)); //...File: script/deploy/Deploy.V2.s.sol
93: vaultLicenser.add(address(ethVault));
94: vaultLicenser.add(address(wstEth));
95:❌ vaultLicenser.add(address(unboundedKerosineVault)); //<@audit: should be added in KerosineManager, not Licenser
96: // vaultLicenser.add(address(boundedKerosineVault));This also make it possible for a user to add these vaults both to their vaults and vaultsKerosene enumerableSet.
Which means that when collateral ratio will be estimated, user WETH and wstETH deposit will be counted as Kerosene and non-Kerosine value, effectively double-counting this collateral value:
File: src/core/VaultManagerV2.sol
230: function collatRatio(
231: uint id
232: )
233: public
234: view
235: returns (uint) {
236: uint _dyad = dyad.mintedDyad(address(this), id);
237: if (_dyad == 0) return type(uint).max;
238: return getTotalUsdValue(id).divWadDown(_dyad);
239: }
240:
241: function getTotalUsdValue(
242: uint id
243: )
244: public
245: view
246: returns (uint) {
247: return getNonKeroseneValue(id) + getKeroseneValue(id); //<@audit: actual deployment script will cause issue here
248: }Loss of funds, user will be able to inflate their collateral value and thus mint more DYAD than system should allow
Manual review
The following diff will not be sufficient to make the whole system function correctly, as there are an issue with how Kerosine price calculate TVL.
Right now, the KerosineManager vaults set holds two functions:
- list licensed kerosine vault
- list assets that are part of the TVL
These two functions have no link, and shouldn't be mixed in the same set. Sponsor also refactor this part and separate this as two sets.
File: script/deploy/Deploy.V2.s.sol:64
KerosineManager kerosineManager = new KerosineManager();
- kerosineManager.add(address(ethVault));
- kerosineManager.add(address(wstEth));
vaultManager.setKeroseneManager(kerosineManager);
kerosineManager.transferOwnership(MAINNET_OWNER);
UnboundedKerosineVault unboundedKerosineVault = new UnboundedKerosineVault(
vaultManager,
Kerosine(MAINNET_KEROSENE),
Dyad (MAINNET_DYAD),
kerosineManager
);
BoundedKerosineVault boundedKerosineVault = new BoundedKerosineVault(
vaultManager,
Kerosine(MAINNET_KEROSENE),
kerosineManager
);
KerosineDenominator kerosineDenominator = new KerosineDenominator(
Kerosine(MAINNET_KEROSENE)
);
unboundedKerosineVault.setDenominator(kerosineDenominator);
unboundedKerosineVault.transferOwnership(MAINNET_OWNER);
boundedKerosineVault. transferOwnership(MAINNET_OWNER);
vaultLicenser.add(address(ethVault));
vaultLicenser.add(address(wstEth));
- vaultLicenser.add(address(unboundedKerosineVault));
+ kerosineManager.add(address(unboundedKerosineVault));
- // vaultLicenser.add(address(boundedKerosineVault));
+ kerosineManager.add(address(boundedKerosineVault));
🔴 H03 - Kerosine price is vulnerable to price manipulation, allowing unfair liquidations and Kerosine price crash
The protocol sets Kerosene’s value as DYAD collateral deterministically (documentation):
The implementation can be found here: https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/Vault.kerosine.unbounded.sol#L65-L66
File: src/core/Vault.kerosine.unbounded.sol
50: function assetPrice()
51: public
52: view
53: override
54: returns (uint) {
55: uint tvl;
56: address[] memory vaults = kerosineManager.getVaults();
57: uint numberOfVaults = vaults.length;
58: for (uint i = 0; i < numberOfVaults; i++) {
59: Vault vault = Vault(vaults[i]);
60: tvl += vault.asset().balanceOf(address(vault))
61: * vault.assetPrice() * 1e18
62: / (10**vault.asset().decimals())
63: / (10**vault.oracle().decimals());
64: }
65: uint numerator = tvl - dyad.totalSupply();
66: uint denominator = kerosineDenominator.denominator();
67: return numerator * 1e8 / denominator;
68: }This price can easily be manipulated by an malicious actor (Alice) with sufficient funds:
- Alice deposit the assets into its dNFT position
tvlwill increase,dyad.totalSupply()will stay the same, sonumeratorwill increase, thus Kerosene pricedenominatorwill not change- Now Alice waits for victims to use Kerosine as collateral for their borrow
- Once Alice see enough interesting positions, she mints DYAD, this will increase
dyad.totalSupply(), thus reducingnumeratorand so Kerosine price - Now, multiple position will become liquidatable (
CR < MIN_COLLATERIZATION_RATIO) - It become even worse now, because each liquidation will reduce
dyad.totalSupply(), causing a liquidation cascade and crashing Kerosine price.
Please note that this effect is proportional to the supply controlled by the whales, but any amount used to operate that exploit will manipulate the price proportionally.
Unfair liquidation and Kerosine price crash
I have setup a test environment based on existing VaultManager.t.sol and DeployBase.s.sol, but had to make some additions to run the tests against VaultManagerV2
Thus will have to add these two files VaultManagerV2.t.sol and DeloyBaseV2.sol, available in the gist that follows: https://gist.github.com/InfectedIsm/111d71fb5265a1f62e3a248c64d5afa0
These files must be added at the same level as VaultManager.t.sol and DeployBase.s.sol respectively.
Output of the test:
Ran 1 test for test/VaultManagerV2.t.sol:VaultManagerV2Test
[PASS] test_ManipulatePrice() (gas: 1046778)
Logs:
ker price: 150000
ker price: 100000
price change: -33%Manual review
Do not use spot value to calculate Kerosine price, but rather an oracle (can be a TWAP based on historic balance/supply) The longer the TWAP window will be chosen, the smoother the price change will be, allowing users to anticipate the manipulation and act accordingly on their position.
🔴 H04 - If protocol TVL drops in value, numerator will underflow in UnboundedKerosineVault::assetPrice
DYAD protocol allow users to deposit collateral and mint a dollar-pegged asset (DYAD) against it. Two types of vaults are proposed:
- exogenous vaults, where users can deposit assets external to the protocol (like WETH, wstETH, ...)
- kerosine vaults, where user can deposit the endogenous token from the protocol, Kerosine
Minting of DYAD tokens require a minimum total (exo + endo) collateralization ratio >150%, and a minimum exo CR100%.
So this assertion should always be verified under good conditions (i.e prices do not changes to quickly): tvl > dyad.totalSupply()
But this is not a realistic that can hold forever.
File: src/core/Vault.kerosine.unbounded.sol
50: function assetPrice()
51: public
52: view
53: override
54: returns (uint) {
55: uint tvl;
56: address[] memory vaults = kerosineManager.getVaults();
57: uint numberOfVaults = vaults.length;
58: for (uint i = 0; i < numberOfVaults; i++) {
59: Vault vault = Vault(vaults[i]);
60: tvl += vault.asset().balanceOf(address(vault))
61: * vault.assetPrice() * 1e18
62: / (10**vault.asset().decimals())
63: / (10**vault.oracle().decimals());
64: }
65:❌ uint numerator = tvl - dyad.totalSupply();//<@audit: this can underflow
66: uint denominator = kerosineDenominator.denominator();
67: return numerator * 1e8 / denominator;
68: }
69: }But if multiple vaults, or a major vault sees its asset price plummet, this relationship will change.
In that case, the substraction will underflow resulting in a revert of assetPrice
The issue is that UnboundedKerosineVault::assetPrice is called by VaultManagerV2::collatRatio >> VaultManagerV2::getTotalUsdValue >> VaultManagerV2::getNonKeroseneValue
And VaultManagerV2::collatRatio is called by VaultManagerV2::liquidate, which is the mechanism expected to clean up bad positions.
File: src/core/VaultManagerV2.sol
205: function liquidate(
206: uint id, //The ID of the dNFT to be liquidated.
207: uint to //The address where the collateral will be sent
208: )
209: external
210: isValidDNft(id)
211: isValidDNft(to)
212: {
213:❌ uint cr = collatRatio(id); //<@audit: this can revert if TVL < dyad.totalSupply()
214: if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
215: dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));
216: If total TVL drops below the minimum CR of the protocol, liquidation will revert, preventing protocol to recover.
Manuel review
Mitigating that issue is difficult as the issue is associated to how Kerosine price is evaluated. This require to refactor how the token is priced, in case of such cases, but this surely shouldn't revert when called during liquidations. Collateralization ratio should also probably be revised to higher values to absorb unexpected dumps in value.
🔴 H05 - Depositing Kerosine into a Vault will DoS all function calling collatRatio because KerosineVault has no oracle
In order to withdraw assets from a vault, the collateral ratio (CR) of the position must be sufficient :
if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); The same checks are made in liquidate and mintDyad
The thing is, VaultManagerV2::collatRatio calls VaultManagerV2::getTotalUsdValue, which calls VaultManagerV2::getKeroseneValue which calls KerosineVault::getUsdValue ultimately calling UnboundedKerosineVault::assetPrice()
File: src/core/Vault.kerosine.unbounded.sol
51: function assetPrice()
52: public
53: view
54: override
55: returns (uint) {
56: uint tvl;
57: address[] memory vaults = kerosineManager.getVaults();
58: uint numberOfVaults = vaults.length;
59: for (uint i = 0; i < numberOfVaults; i++) {
60: Vault vault = Vault(vaults[i]);
61: tvl += vault.asset().balanceOf(address(vault))
62: * vault.assetPrice() * 1e18
63: / (10**vault.asset().decimals())
64: / (10**vault.oracle().decimals());
65: }
66: uint numerator = tvl - dyad.totalSupply();
67: uint denominator = kerosineDenominator.denominator();
68: return numerator * 1e8 / denominator;
69: }But neither this contract, or KerosineVault (which it inherit from) has an oracle getter to call.
Functions calling collatRatio() are: liquidate, withdraw and mintDyad
This means user depositing Kerosine will see all their asset stuck in the vault.
But not only that, positions cannot be liquidated until the problem is solved (by redeploying a new KerosineVault with an oracle)
Manual review
Add an oracle to the KerosineVault (or UnboundedKerosineVault)
abstract contract KerosineVault is IVault, Owned(msg.sender) {
using SafeTransferLib for ERC20;
IVaultManager public immutable vaultManager;
ERC20 public immutable asset;
KerosineManager public immutable kerosineManager; //@audit unused state variable
+ IAggregatorV3 public immutable oracle;
mapping(uint => uint) public id2asset;https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/VaultManagerV2.sol#L143-L143 https://github.com/code-423n4/2024-04-dyad/blob/49fb7174576e5147dc73d3222d16954aa641a2e0/src/core/VaultManagerV2.sol#L127-L127
User depositing in vault A will not be able to withdraw from vault B in the same block. That protection has been implemented to protect DYAD from price update front-running issues, so this should be specific to a vault and not to a dNFT position.
File: src/core/VaultManagerV2.sol
119: function deposit(
120: uint id,
121: address vault,
122: uint amount
123: )
124: external
125: isValidDNft(id)
126: {
127: idToBlockOfLastDeposit[id] = block.number;File: src/core/VaultManagerV2.sol
134: function withdraw(
135: uint id,
136: address vault,
137: uint amount,
138: address to
139: )
140: public
141: isDNftOwner(id)
142: {
143: if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();This make it impossible for a user or external strategies that might want to interact with DYAD to rebalance collateral in one transaction.
Manual review
Add vault as a parameter to idToBlockOfLastDeposit[id] mapping, e.g: idToBlockOfLastDeposit[id][vault]
This will prevent oracle front-running, while still allowing callers to rebalance collateral in one tx.
Right now there are no incentives to liquidate a position with a CR<1, as the liquidator will have to burn the full borrowed amount, and will get the full collateral.
But a CR<1 means collateral is worth less than borrowed amount.
So this is a clear loss for the liquidator, meaning no-one will liquidate the position.
File: src/core/VaultManagerV2.sol
205: function liquidate(
206: uint id, //The ID of the dNFT to be liquidated.
207: uint to //The address where the collateral will be sent
208: )
...: // ... some code ...
215:❌ dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id)); //<@audit: caller need to burn full borrowed amount
216:
217: uint cappedCr = cr < 1e18 ? 1e18 : cr; /// == max(1e18, cr)
218: uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD); /// if cr<1, this is equal 0
219: uint liquidationAssetShare = (liquidationEquityShare + 1e18).divWadDown(cappedCr); /// if cr<1, this is equal 1e18
220:
221: uint numberOfVaults = vaults[id].length();
222: for (uint i = 0; i < numberOfVaults; i++) {
223: Vault vault = Vault(vaults[id].at(i));
224: uint collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
225: vault.move(id, to, collateral);
226: }
227: emit Liquidate(id, msg.sender, to);
228: }If CR<1, position will not be liquidated, protocol possibly incuring a worser bad debt than this could have been.
Manuel review
Refactor the liquidation calculation, to allow liquidator to repay the debt and still get a reward out of this.
There is no slippage/deadline parameter available when redeeming DYAD against collateral.
File: src/core/VaultManagerV2.sol
184: function redeemDyad(
185: uint id,
186: address vault,
187: uint amount,
188: address to
189: )
190: external
191: isDNftOwner(id)
192: returns (uint) {
193: dyad.burn(id, msg.sender, amount);
194: Vault _vault = Vault(vault);
195: uint asset = amount
196:❌ * (10**(_vault.oracle().decimals() + _vault.asset().decimals())) //<@audit: vulnerable to slippage
197: / _vault.assetPrice()
198: / 1e18;
199: withdraw(id, vault, asset, to);
200: emit RedeemDyad(id, vault, amount, to);
201: return asset;
202: }But withdrawn assets are dependent on the oracle price, which cannot be predicted by user depending on the time the tx will be added to a block.
User is vulnerable to slippage and can experience unexpected trading prices against its DYAD
Manual review
Add a minAssetOut parameter (I haven't added the deadline parameter but this is a possibility)
File: src/core/VaultManagerV2.sol:184
function redeemDyad(
uint id,
address vault,
uint amount,
+ uint minAssetOut,
address to
)
external
isDNftOwner(id)
returns (uint) {
dyad.burn(id, msg.sender, amount);
Vault _vault = Vault(vault);
uint asset = amount
* (10**(_vault.oracle().decimals() + _vault.asset().decimals()))
/ _vault.assetPrice()
/ 1e18;
+ require(asset >= minAssetOut, "received amount too low");
withdraw(id, vault, asset, to);
emit RedeemDyad(id, vault, amount, to);
return asset;
}boundedKerosineVault is deployed, but the setUnboundedKerosenVault is not called, which will cause a revert when BounderKerosineVault::assetPrice() will be called to price users kerosine collateral:
File: src/core/Vault.kerosine.bounded.sol
22:
23: function setUnboundedKerosineVault(
24: UnboundedKerosineVault _unboundedKerosineVault
25: )
26: external
27: onlyOwner
28: {
29: unboundedKerosineVault = _unboundedKerosineVault;
30: }
31:
...:
...: /// ... some code ...
...:
44: function assetPrice()
45: public
46: view
47: override
48: returns (uint) {
49: return unboundedKerosineVault.assetPrice() * 2;
50: }File: src/core/Vault.sol
14: contract Vault is IVault {
15: using SafeTransferLib for ERC20;
16: using SafeCast for int;
17: using FixedPointMathLib for uint;
18:
19: uint public constant STALE_DATA_TIMEOUT = 90 minutes; Each vault uses a chainlink oracle to access the underlying asset price. While it check for stale data as expected, the delay is a constant that cannot be set when deploying a new vault. This is not correct, as depending on the oracle, the timeout to consider an answer as stale can be different.
Make STALE_DATA_TIMEOUT a constructor parameter, or add a setter function.
File: src/core/Vault.sol
091: function assetPrice()
092: public
093: view
094: returns (uint) {
095: (
096: ,
097: int256 answer,
098: ,
099: uint256 updatedAt,
100: ) = oracle.latestRoundData();
101: if (block.timestamp > updatedAt + STALE_DATA_TIMEOUT) revert StaleData();
102: return answer.toUint256();
103: }Its common practice to make oracle calls inside a try/catch structure in case there's an issue with the oracle making it revert.
The issue is that withdrawal/liquidation in VaultManagerV2 could be prevented in that case.
Part of this common practice is to have a fallback oracle (TWAP) or use a saved price (not the best choice though)
Price feed might return zero and this must be handled as invalid.
File: src/core/Vault.sol
091: function assetPrice()
092: public
093: view
094: returns (uint) {
095: (
096: ,
097: int256 answer,
098: ,
099: uint256 updatedAt,
100: ) = oracle.latestRoundData();
101: if (block.timestamp > updatedAt + STALE_DATA_TIMEOUT) revert StaleData();
102: return answer.toUint256();
103: }Ensure the returned price is not zero and act accordingly depending on your design choices.