search

code-423n4 / 2021-09-yaxis-findings

0 stars 0 forks source link

`Vault.withdraw` sometimes burns too many shares #121

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

cmichel

Vulnerability details

The Vault.withdraw function attempts to withdraw funds from the controller if there are not enough in the vault already. In the case the controller could not withdraw enough, i.e., where _diff < _toWithdraw, the user will receive less output tokens than their fair share would entitle them to (the initial _amount).

if (_diff < _toWithdraw) {
    // @audit burns too many shares for a below fair-share amount
    _amount = _balance.add(_diff);
}

Impact

The withdrawer receives fewer output tokens than they were entitled to.

Recommended Mitigation Steps

In the mentioned case, the _shares should be recomputed to match the actual withdrawn _amount tokens:

if (_diff < _toWithdraw) {
    _amount = _balance.add(_diff);
    // recompute _shares to burn based on the lower payout
    // should be something like this, better to cache balance() once at the start and use that cached value
    _shares = (totalSupply().mul(_amount)).div(_balance);
}

Only these shares should then be burned.

uN2RVw5q commented 2 years ago

Duplicate of https://github.com/code-423n4/2021-09-yaxis-findings/issues/41 and https://github.com/code-423n4/2021-09-yaxis-findings/issues/136

GalloDaSballo commented 2 years ago

Agree with the finding

Anytime the strategy incurs a loss during withdrawal, the person that triggered that withdrawal will get less for their shares than what they may expect.

Since amount of shares is computed by checking balance in strategy, and controller enacts this withdrawal, adding a check in the controller to compare expected withdrawal vs actual shares received would be a clean way to mitigate