Description

The WasabiVault::_withdraw function does not follow the checks-effects-interactions pattern, and contains a potential reentrancy issue.

function _withdraw(
    address caller,
    address receiver,
    address owner,
    uint256 assets,
    uint256 shares
) internal virtual override {
    if (caller != owner) {
        _spendAllowance(owner, caller, shares);
    }

    _burn(owner, shares);
!   pool.withdraw(asset(), assets, receiver);

!   totalAssetValue -= assets;

    emit Withdraw(caller, receiver, owner, assets, shares);
}

The totalAssetValue variable is updated only after pool.withdraw(...) is invoked.

Impact

Assuming only legitimate ERC20 assets are used, and therefore that the attacker cannot gain control during the call to pool.withdraw(...), this issue does not constitute an exploitable vulnerability, and as such is reported as informational.

However, careful vetting of the allowed ERC20 assets is required unless a code change is made to mitigate the issue.

Recommendations

Strictly follow the checks-effects-interactions pattern, updating totalAssetValue before invoking pool.withdraw(...).

Consider adding reentrancy protection to the contract, using the nonReentrant modifier on all functions that could be exploited if reentered. All public functions should ideally be protected, unless reentrancy is explicitly desired.

Remediation

This issue has been acknowledged by Wasabi, and a fix was implemented in commit a5b73f2f↗.