Incorrect implementation of USPC._checkMinShares causes DOS.
Description
In the USPC contract, the _checkMinShares function guarantees that after all deposits and withdrawals, if _totalSupply is not equal to 0 or is less than MIN_SHARES, then it will revert:
/**
* @notice Internal deposit function with minimum shares check
* @dev Overrides ERC4626 _deposit to add donation attack protection
* @inheritdoc ERC4626Upgradeable
*/
function _deposit(address _caller, address _receiver, uint256 _assets, uint256 _shares)
internal
override
whenNotPaused
{
super._deposit(_caller, _receiver, _assets, _shares);
_checkMinShares();
}
// [...]
/**
* @notice Internal withdraw function with minimum shares check
* @dev Overrides ERC4626 _withdraw to add donation attack protection
* @dev Throws if owner, caller, or receiver is not allowlisted
* @inheritdoc ERC4626Upgradeable
*/
function _withdraw(address _caller, address _receiver, address _owner, uint256 _assets, uint256 _shares)
internal
override
whenNotPaused
{
require(isAllowlisted(_owner), OwnerNotAllowlisted());
require(_owner == _caller || isAllowlisted(_caller), CallerNotAllowlisted());
require(_owner == _receiver || _caller == _receiver || isAllowlisted(_receiver), ReceiverNotAllowlisted());
super._withdraw(_caller, _receiver, _owner, _assets, _shares);
_checkMinShares();
}
// [...]
/**
* @notice Ensures a small non-zero amount of shares does not remain, exposing to donation attack
* @dev Prevents attackers from donating small amounts to manipulate share price
*/
function _checkMinShares() internal view {
uint256 _totalSupply = totalSupply();
require(_totalSupply == 0 || _totalSupply >= MIN_SHARES, MinSharesViolation()); // @audit: revert if `0 < _totalSupply < 10000e6`
}This introduces the possibility of a DOS attack.
Impact
Consider the following scenario:
User1 deposits
10000e6USDC and receives10000e6shares.The attacker deposits 1 wei.
User1 executes the
_withdrawfunction to get10000e6USDC. At this point,_totalSupplyis 1 wei, causingrequire(_totalSupply == 0 || _totalSupply >= MIN_SHARES, MinSharesViolation());to revert.
In this case, attackers can prevent users from withdrawing at extremely low cost.(1 wei)
Recommendations
Consider defending with a virtual offset↗
Use an offset between the "precision" of the representation of shares and assets. Said otherwise, we use more decimal places to represent the shares than the underlying token does to represent the assets.
Include virtual shares and virtual assets in the exchange rate computation. These virtual assets enforce the conversion rate when the vault is empty.
Remediation
This issue has been acknowledged by Coinshift, and a fix was implemented in commit d14d8915↗.