c4-bot-7 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

By providing empty deposits to ids of other users, legit users of the protocol can get their withdrawal and redeem in the VaultManagerV2 reverted by a malicious user. Apart from the grief this would cause to users and the protocol, users may also be external protocols or smart contract systems which rely on smooth functionality of the dyad system. Note that the malicious user can DOS withdrawal and redeem for every single user available and for every block. The user only has to worry about gas fees. This would be especially easier in the early adoption of the protocol with relatively less users.

Proof of Concept

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

Due to lack of access control on the VaultManager.deposit function, users can make deposits into any DyadNFT for any vault. This seems to be a system design that allows users to make payments for other users.

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

Additionally, the block.number at that deposit is set to idToBlockOfLastDeposit[id] for that id. This is to prevent users from withdrawing within the same block as their last deposit, also this check is indirectly enforced during redeem as withdrawal happens during the redeem process.

function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
 @>   if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock

function redeemDyad(
    uint    id,
    address vault,
    uint    amount,
    address to
  )
    external 
      isDNftOwner(id)
    returns (uint) { 
      ...............
     withdraw(id, vault, asset, to);

This is where the malicious user can grief legit users The malicious user can frontrun any withdrawal or redeem in the mempool with a 0 amount deposit into the id of the user, causing the lastdeposit for that id to be reset to that block.number. This would subsequently cause the withdrawal or redeem for the unsuspecting user to revert due to the check. The malicious user can do this for the next block and every single block as long as they intend. Apart from griefing,the bad actor may have other legitimate reasons for doing this and create more attack paths. For example, if the legit user is an external protocol, the bad actor can prevent their withdrawal in order to build an attack on that protocol.

Tools Used

Manual review

Recommended Mitigation Steps

A mitigation step would be to dissalow 0 amount deposits or set a minimum deposit such that the bad actor is discouraged due to financial losses.


function deposit(
uint    id,
address vault,
uint    amount
) 
external 
  isValidDNft(id)
{
+  require(amount > 0 /** || amount > minDeposit*/);
idToBlockOfLastDeposit[id] = block.number;
Vault _vault = Vault(vault);
_vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
_vault.deposit(id, amount);
}



## Assessed type

DoS

c4-pre-sort commented 6 months ago

JustDravee marked the issue as duplicate of #1103

c4-pre-sort commented 6 months ago

JustDravee marked the issue as duplicate of #489

c4-pre-sort commented 6 months ago

JustDravee marked the issue as sufficient quality report