Attacker can make 0 value deposit() calls to deny user from redeeming or withdrawing collateral

criticalCode4rena

Lines of code

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

Vulnerability details

Summary

Root cause: Function deposit() uses modifier isValidDNft() instead of isDNftOwner(), which allows anyone to call deposit() on behalf of any DNft id.

Impacts:

1. Attacker can make user devoid of withdrawing or redeeming collateral by making 0 value deposit() calls. Other than denying users from temporarily withdrawing their collateral, this is also an issue since it could force users into liquidations when they try to take preventative measures on their collateral ratio (especially through redeemDyad()) in high collateral price volatility situations. If successful, the attacker could then perform the liquidation to profit from the situation.
2. Attacker can make user devoid of removing vault due to id2asset > 0 by depositing 1 wei of collateral.

Proof of Concept

Let's understand the first impact:

1. User calls redeemDyad() (or withdraw() to directly withdraw collateral) to burn DYAD and withdraw their collateral asset.

2. Attacker frontruns the call with a 0 value deposit() call. This would set the idToBlockOfLastDeposit[id] to the current block.number. The call does not revert since 0 value transfers are allowed.

solidity📋
File: VaultManagerV2.sol
127:   function deposit(
128:     uint    id,
129:     address vault,
130:     uint    amount
131:   ) 
132:     external 
133:       isValidDNft(id)
134:   {
135:     idToBlockOfLastDeposit[id] = block.number;
136:     Vault _vault = Vault(vault);
137:     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
138:     _vault.deposit(id, amount);
139:   }

3. When the user's redeemDyad() call goes through, it internally calls the withdraw() function, which would cause a revert due to the check on Line 152. The check evaluates to true and reverts since the attacker changes the last deposit block number to the current block through the 0 value deposit() call.

solidity📋
File: VaultManagerV2.sol
143:   function withdraw(
144:     uint    id,
145:     address vault,
146:     uint    amount,
147:     address to
148:   ) 
149:     public
150:       isDNftOwner(id)
151:   {
152:     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
153:     uint dyadMinted = dyad.mintedDyad(address(this), id);
154:     Vault _vault = Vault(vault);
155:     uint value = amount * _vault.assetPrice() 
156:                   * 1e18 
157:                   / 10**_vault.oracle().decimals() 
158:                   / 10**_vault.asset().decimals();
159:  
160:     if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
161:     _vault.withdraw(id, to, amount);
162:     if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
163:   }

4. If the collateral ratio of the user falls below the minimum threshold of 1.5e18 (in terms of high volatility of collateral asset price), the attacker could then exploit the situation to liquidate the user using the liquidate() function.

Let's understand the second impact:

1. User tries to remove a vault by calling the remove() function.

2. Attacker frontruns the call by making a 1 wei collateral deposit through the deposit() function. This would increase the id2asset for the user in the vault.

solidity📋
File: VaultManagerV2.sol
127:   function deposit(
128:     uint    id,
129:     address vault,
130:     uint    amount
131:   ) 
132:     external 
133:       isValidDNft(id)
134:   {
135:     idToBlockOfLastDeposit[id] = block.number;
136:     Vault _vault = Vault(vault);
137:     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
138:     _vault.deposit(id, amount);
139:   }

3. User's call goes through and reverts due to the check on Line 102. This revert occurs since id2asset is now 1 wei for the vault the user is trying to remove. Note that although the attacker would be spending gas here, an equal amount of gas would also be required from the user's side to withdraw the 1 wei. The attack will continue till the user gives up due to the high gas spent behind withdrawing. Another thing to note is that regular users (with no knowledge of contracts) might not have the option to withdraw 1 wei from the frontend, which would require additional overhead from their side to seek help from the team.

solidity📋
File: VaultManagerV2.sol
095:   function remove(
096:       uint    id,
097:       address vault
098:   ) 
099:     external
100:       isDNftOwner(id)
101:   {
102:     if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
103:     if (!vaults[id].remove(vault))     revert VaultNotAdded();
104:     emit Removed(id, vault);
105:   }

Tools Used

Manual Review

Recommended Mitigation Steps

Use modifier isDNftOwner() instead of isValidDNft() on function deposit().

Assessed type

Invalid Validation