Description

The shares minted in deposit() are calculated as a ratio of totalVaultFunds() and totalSupply(). The totalVaultFunds() can be potentially inflated, reducing the amounts of shares minted (even zero).

    function deposit(uint256 amountIn, address receiver)
    ...
        shares = totalSupply() > 0
            ? (totalSupply() * amountIn) / totalVaultFunds()
            : amountIn;
        IERC20(wantToken).safeTransferFrom(receiver, address(this), amountIn);
        _mint(receiver, shares);
    }
...

    function totalVaultFunds() public view returns (uint256) {
        return
            IERC20(wantToken).balanceOf(address(this)) + totalExecutorFunds();
    }

By transferring wantToken tokens directly, totalVaultFunds() would be inflated (because of balanceOf()) and as the division result is floored, there could be a case when it would essentially mint zero shares, causing a loss for the depositing user. If an attacker controls all of the share supply before the deposit, they would be able to withdraw all the user-deposited tokens.

Impact

Consider the following attack scenario:

1. The Vault contract is deployed.

2. The governance sets batcherOnlyDeposit to false.

3. The attacker deposits X stakeable tokens and receives X LP tokens.

4. The victim tries to deposit Y stakeable tokens.

5. The attacker frontruns the victim's transaction and transfers X * (Y - 1) + 1 stakeable tokens to the Vault contract.

6. The victim's transaction is executed, and the victim receives 0 LP tokens.

7. The attacker redeems her LP tokens, effectively stealing Y stakeable tokens from the victim.

The foregoing is just an example. Variations of the foregoing attack scenario are possible.

The impact of this finding is mitigated by the fact that the default value of batcherOnlyDeposit is true, which allows the keeper of the Batcher contract to 1) prevent the attacker from acquiring 100% of the total supply of LP tokens and 2) prevent the attacker from redeeming their LP tokens for stakeable tokens.

Recommendations

Consider

• adding an amountOutMin parameter to the deposit(uint256 amountIn, address receiver) function of the Vault contract;

• adding a require statement that ensures that the deposit() function never mints zero or less than amountOutMin LP tokens.

Remediation

The issue has been acknowledged by Brahma and mitigated in commit 413b9cc↗.