kutugu - _account_for_withdraw_liquidity rounding in the wrong direction will run out of the vault
Opened this issue · 2 comments
kutugu
high
_account_for_withdraw_liquidity rounding in the wrong direction will run out of the vault
Summary
Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users：
If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.
If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.
When the user extracts liquidity, due to the existence of share, the calculation has precision errors, should pay special attention to the rounding direction. Users withdraw a specific amount of liquidity, user shares should be rounded up, subtract 1 more shares, to avoid spending more than they earn, resulting in the eventual run out of the vault.
Vulnerability Detail
@internal
def _account_for_withdraw_liquidity(
_token: address, _amount: uint256, _is_safety_module: bool
):
self._update_debt(_token)
if _is_safety_module:
shares: uint256 = self._amount_to_lp_shares(_token, _amount, True)
assert (shares <= self.safety_module_lp_shares[_token][msg.sender]), "cannot withdraw more than you own"
self.safety_module_lp_total_amount[_token] -= _amount
self.safety_module_lp_total_shares[_token] -= shares
self.safety_module_lp_shares[_token][msg.sender] -= shares
else:
shares: uint256 = self._amount_to_lp_shares(_token, _amount, False)
assert (shares <= self.base_lp_shares[_token][msg.sender]), "cannot withdraw more than you own"
self.base_lp_total_amount[_token] -= _amount
self.base_lp_total_shares[_token] -= shares
self.base_lp_shares[_token][msg.sender] -= shares
@internal
@view
def _amount_to_lp_shares(
_token: address, _amount: uint256, _is_safety_module: bool
) -> uint256:
if _is_safety_module:
# initial shares == wei
if self.safety_module_lp_total_shares[_token] == 0:
return _amount * PRECISION
wei_per_share: uint256 = self._amount_per_safety_module_lp_share(_token)
new_shares: uint256 = _amount * PRECISION * PRECISION / wei_per_share
return new_shares
else: # base_lp
# initial shares == wei
if self.base_lp_total_shares[_token] == 0:
return _amount * PRECISION
wei_per_share: uint256 = self._amount_per_base_lp_share(_token)
new_shares: uint256 = _amount * PRECISION * PRECISION / wei_per_share
return new_shares_amount_to_lp_shares calculates the amount of share to be subtracted, which should round up, otherwise the user account share will be 1 more than it actually is, and the difference will gradually become larger as users interact with vault over time, eventually run out of the vault.
Impact
Rounding in the wrong direction, when users withdraw liquidity, the shares are greater than actual value, and gradually accumulate as interactions, eventually causing the vault to be run out of and unable to repay all the liquidity.
Code Snippet
Tool used
Manual Review
Recommendation
Round up in _amount_to_lp_shares
Fixed by subtracting a share from the calculation of self._amount_to_lp_shares to account for the exposed rounding error