c4-bot-4 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/59b1f70cbf170871f9604e73e7fe70b70981ab43/contracts/core/Vault.sol#L212

Vulnerability details

Impact

DoS attacks make it impossible for others to deposit

Proof of Concept

maxDeposit is set in the deposit function:

    if (maxDeposit > 0) {
        uint256 afterDeposit = msg.value + ((balanceOf(msg.sender) * _tokenPerETH(maxPriceAge)) / 1e18);
        if (afterDeposit > maxDeposit) revert MaxDepositReached();
    }

_tokenPerETH = (totalSupply() * 1 ether) / _totalAssets;

Because _totalAssets = totalCollateralInEth - totalDebtInEth So the value of _totalAssets might be small.

Suppose _totalAssets = 1wei, afterDeposit = msg.value + balanceOf(msg.sender) * totalSupply()

Since totalSupply is constantly increasing, Therefore, when the value of totalSupply is accumulated (many deposits in a short time) and _totalAssets in the pool is small, a user with balanceOf(msg.sender) greater than 0 May not be able to deposit.

The attacker can make totalSupply increase temporarily, or the attacker can donate the balance to the specified account so that his balance is greater than 0.

Tools Used

vscode, manual

Recommended Mitigation Steps

Change the calculation method of maxDeposit.

Assessed type

DoS

stalinMacias commented 5 months ago

The points used on this report to explain why a deposit can be DoS seems to be off and wrong.

"The attacker can make totalSupply increase temporarily"
- Exactly, how an attacker would do this? totalSupply is in relation to the amount of existing shares for the total deposits made on the vault. How an attacker would increase temporarely the total supply without doing a deposit?
"the attacker can donate the balance to the specified account so that his balance is greater than 0."
- The "attacker" would basically gift shares that are redeemable for ETH to the "victim". Plus, donating shares to an account doesn't cause a DoS, unless the "attacker" "donates" enough shares that the user's balance hits the limits. Which, if this happens:
  1. The "victim" basically received for free shares that are claimable for ETH. The "victim" proceeds to withdraw those shares and then it can deposit more ETH.

This report fails to mention the root cause of why deposits could be DoSed by donating the underling aCollateralToken to the Vault when there are no existing shares.

For the above reason, this report should not be a dupe of the reports that correctly points out the root cause of DoSing deposits, and this report seems to be invalid.

c4-judge commented 5 months ago

0xleastwood marked the issue as duplicate of #29