Description

The BeefyVaultConcLiq contract's withdraw function allows holders of the vault's shares to redeem them against the two tokens. The function calculates the amounts of the two tokens the users should be transferred, _amount0 and _amount1, and obtains the balances before0 and before1 that the vault contract currently holds in these tokens. During normal operations, these values would be zero (see also ref↗). If the balance the vault holds is not enough to redeem the shares, the necessary tokens are withdrawn from the strategy contract. This is done starting from line 227:

if (before0 < _amount0 ||  before1 < _amount1) {

    uint _withdraw0 = _amount0 - before0;
    uint _withdraw1 = _amount1 - before1;

    strategy.withdraw(_withdraw0, _withdraw1);

This code will revert due to an arithmetic underflow if the branch is taken because one of the before values is smaller than the corresponding _amount value, but the other before value is larger than the corresponding _amount value (for example, if before0 < _amount0 but before1 < _amount1).

Impact

Assume, as will be the case during normal operations, that the vault does not hold any of token0 or token1. An attacker could transfer some amount of one token, say x of token0, to the vault. Withdrawals of amounts of token0 smaller than x would then revert. This issue thus allows a denial-of-service attack on withdrawals, albeit one at a potentially significant cost to the attacker.

Recommendations

Consider replacing

uint _withdraw0 = _amount0 - before0;

by

uint _withdraw0;
if (_amount0 >= before0) _withdraw0 = _amount0 - before0;

to make _withdraw0 zero when _amount0 < before0, and analogously for _withdraw1.

Alternatively, given that the vault should not hold tokens during normal operations (see also ref↗), the logic in withdraw could be simplified by always withdrawing the full amounts needed from the strategy contract and not using any tokens sent to the vault directly. This would mean, however, that any token0 or token1 sent to the vault accidentally would be stuck instead of being treated as a donation and used on withdrawals.

Remediation

This issue has been acknowledged by Beefy, and a fix was implemented in commit 8ff8e999↗.