Assessment reports>Beefy UniswapV3>Discussion>Vault withdraw simplification

The vault's withdraw function could be simplified

The BeefyVaultConcLiq contract contains the following piece of code from line 226 to 240:

(uint before0, uint before1)  = available();
if (before0 < _amount0 ||  before1 < _amount1) {

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

    strategy.withdraw(_withdraw0, _withdraw1);

    (uint after0, uint after1) = available();
    uint diff0 = after0 - before0;
    uint diff1 = after1 - before1;
    
    if (diff0 < _withdraw0) _amount0 = before0 + diff0;
    if (diff1 < _withdraw1) _amount1 = before1 + diff1;
}

This code snippet could be simplified.

As diff0 and diff1 are not used after the the two ifs directly after their definition, their definition could be removed and the two ifs near the end replaced by the equivalent following lines.

if(_after0 < _amount0) _amount0 = after0;
if(_after1 < _amount1) _amount1 = after1;

If the strategy contract used is StrategyPassiveManagerUniswap, then we will always have diff0 = _withdraw0 and diff1 = _withdraw1 (see Finding ref), so these lines could be removed completely as well.

More generally, during normal operation, the vault only obtains tokens temporarily exactly due to the strategy.withdraw(_withdraw0, _withdraw1) call, and those tokens are then (assuming the vault's previous balance was zero) directly transferred to the caller. Outside of calls to the vault's withdraw function, the vault would thus usually not hold any balance of token0 or token1 and could only do so if sent tokens directly. Given this, one could consider to simplify the entire piece of code quoted above by replacing it with

strategy.withdraw(_amount0, _amount1);

During normal operations with the StrategyPassiveManagerUniswap contract as strategy contract, this would make the withdraw function functionally equivalent to the previous version, with the exception of Findings ref and ref having been fixed. There would be one noticeable difference in functionality: should users, perhaps accidentally, send tokens to the vault, then the previous version would treat those like a donation. This replacement would instead not make use of these tokens when paying out withdrawals. These tokens would instead be stuck. In order to avoid such tokens influencing the price of vault shares, one should then not count them anymore in balances().

Zellic © 2024Back to top ↑