User token withdraws from vaults can be griefed

[thebrittfactor]

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L134-L153
https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L119-L131

Vulnerability details

Impact

The withdraw() function in implements the following check:

if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock()

not allowing withdraws in the same block, in which someone has already deposited into a so called Note. As can be seen from the deposit() function:

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

There are no minimum deposits required, so a malicious actor can frontrun the Note owner call to withdraw(), deposit as little as 1 WEI and the withdraw function will revert, thus griefing the Note owner. A malicious actor can do this as many times as he wants, and the Note owner won't be able to withdraw his deposited tokens. In a turbulent market conditions (which are a typical occurrence in crypto), a Note owner may have decided that he would like to convert his crypto assets to fiat currency, already burned all of his minted DYAD tokens in order to free all of his collateral. However as he is trying to withdraw all of his deposited collateral, a malicious actor can grief him as described above, while the deposited collateral falls in value. Thus resulting in immense losses for the owner of the Note. The same griefing attack can be performed when a user is trying to remove a vault from his vaults, or vaultsKerosene lists. For example he has liquidated an undercollateralized position of somebody, he already has 5 vaults in his vaults list but a big portion of the collateral he received as a reward are in a vault he hasn't added to his vaults list. In order to withdrawn them he will have to remove one vault and add another, however a malicious user can keep depositing 1 WEI, thus restricting this functionality. Considering when a positions gets liquidated, it is typically because the collateral asset fell in dollar value, and is probably going to continue falling in price due to some turbulent market conditions, this may result in a big loss for users.

Tools Used

Manual review

Recommended Mitigation Steps

Consider allowing only the DNFT Owner to be able to deposit assets via the deposit() function, set the modifier from isValidDNft(id) to isDNftOwner(id)

Assessed type

DoS

[thebrittfactor]

For transparency, the judge has requested that issue #177 be duplicated, as it contains two issues they deemed should be judged separately.

[c4-judge]

koolexcrypto marked the issue as duplicate of #118

[c4-judge]

koolexcrypto marked the issue as satisfactory

[koolexcrypto]

split from #177

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)