Skip to content

Commit

Permalink
Merge pull request #18 from GenerationSoftware/gen-228-c4-issue-143
Browse files Browse the repository at this point in the history 
fix(exchangeRate): add 1:1 exchange rate
  • Loading branch information
@PierrickGT
PierrickGT authored Aug 16, 2023
2 parents c1ec4dc + 81fe36a commit 1017a67
Show file tree
Hide file tree
Showing 10 changed files with 639 additions and 466 deletions.
548 changes: 275 additions & 273 deletions lcov.info
View file

Large diffs are not rendered by default.

150 changes: 64 additions & 86 deletions src/Vault.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ error MintMoreThanMax(address receiver, uint256 shares, uint256 max);
/// @notice Emitted when `_deposit` is called but no shares are minted back to the receiver.
error MintZeroShares();

/// @notice Emitted when `_withdraw` is called but no assets are being withdrawn.
error WithdrawZeroAssets();

/// @notice Emitted when `sweep` is called but no underlying assets are currently held by the Vault.
error SweepZeroAssets();

Expand Down Expand Up @@ -102,9 +105,19 @@ error LiquidationAmountOutZero();
*/
error LiquidationAmountOutGTYield(uint256 amountOut, uint256 availableYield);

/// @notice Emitted when the vault is under-collateralized.
/// @notice Emitted when the Vault is under-collateralized.
error VaultUnderCollateralized();

/**
* @notice Emitted when after a deposit the amount of withdrawable assets from the YieldVault is lower than the expected amount.
* @param withdrawableAssets The actual amount of assets withdrawable from the YieldVault
* @param expectedWithdrawableAssets The expected amount of assets withdrawable from the YieldVault
*/
error YVWithdrawableAssetsLTExpected(
uint256 withdrawableAssets,
uint256 expectedWithdrawableAssets
);

/**
* @notice Emitted when the target token is not supported for a given token address.
* @param token The unsupported token address
Expand Down Expand Up @@ -243,13 +256,6 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
*/
event Sweep(address indexed caller, uint256 assets);

/**
* @notice Emitted when the `_lastRecordedExchangeRate` is updated.
* @param exchangeRate The recorded exchange rate
* @dev This happens on mint and burn of shares
*/
event RecordedExchangeRate(uint256 exchangeRate);

/* ============ Variables ============ */

/// @notice Address of the TwabController used to keep track of balances.
Expand All @@ -267,12 +273,6 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
/// @notice Address of the ILiquidationPair used to liquidate yield for prize token.
ILiquidationPair private _liquidationPair;

/// @notice Underlying asset unit (i.e. 10 ** 18 for DAI).
uint256 private _assetUnit;

/// @notice Most recent exchange rate recorded when burning or minting Vault shares.
uint256 private _lastRecordedExchangeRate;

/// @notice Yield fee percentage represented in integer format with 9 decimal places (i.e. 10000000 = 0.01 = 1%).
uint256 private _yieldFeePercentage;

Expand Down Expand Up @@ -331,8 +331,6 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
_setYieldFeeRecipient(yieldFeeRecipient_);
_setYieldFeePercentage(yieldFeePercentage_);

_assetUnit = 10 ** super.decimals();

// Approve once for max amount
asset_.safeApprove(address(yieldVault_), type(uint256).max);

Expand Down Expand Up @@ -409,15 +407,6 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
return _totalSupply();
}

/**
* @notice Current exchange rate between the Vault shares and
* the total amount of underlying assets withdrawable from the YieldVault.
* @return uint256 Current exchange rate
*/
function exchangeRate() public view returns (uint256) {
return _currentExchangeRate();
}

/**
* @notice Check if the Vault is collateralized.
* @return bool True if the vault is collateralized, false otherwise
Expand Down Expand Up @@ -480,6 +469,8 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {

/// @inheritdoc ERC4626
function deposit(uint256 _assets, address _receiver) public virtual override returns (uint256) {
_requireVaultCollateralized();

if (_assets > maxDeposit(_receiver))
revert DepositMoreThanMax(_receiver, _assets, maxDeposit(_receiver));

Expand Down Expand Up @@ -513,6 +504,8 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {

/// @inheritdoc ERC4626
function mint(uint256 _shares, address _receiver) public virtual override returns (uint256) {
_requireVaultCollateralized();

uint256 _assets = _convertToAssets(_shares, Math.Rounding.Up);

_deposit(msg.sender, _receiver, _assets, _shares);
Expand Down Expand Up @@ -911,49 +904,42 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
* @inheritdoc ERC4626
* @param _assets Amount of assets to convert
* @param _rounding Rounding mode (i.e. down or up)
* @return uint256 Amount of shares for the assets
* @return uint256 Amount of shares corresponding to the assets
*/
function _convertToShares(
uint256 _assets,
Math.Rounding _rounding
) internal view virtual override returns (uint256) {
uint256 _exchangeRate = _currentExchangeRate();
uint256 _collateralAssets = _collateral();
uint256 _depositedAssets = _totalSupply();

if (_assets == 0 || _depositedAssets == 0) {
return _assets;
}

return
(_assets == 0 || _exchangeRate == 0)
? _assets
: _assets.mulDiv(_assetUnit, _exchangeRate, _rounding);
_collateralAssets == 0 ? 0 : _assets.mulDiv(_depositedAssets, _collateralAssets, _rounding);
}

/**
* @inheritdoc ERC4626
* @param _shares Amount of shares to convert
* @param _rounding Rounding mode (i.e. down or up)
* @return uint256 Amount of assets represented by the shares
* @return uint256 Amount of assets corresponding to the shares
*/
function _convertToAssets(
uint256 _shares,
Math.Rounding _rounding
) internal view virtual override returns (uint256) {
return _convertToAssets(_shares, _currentExchangeRate(), _rounding);
}
uint256 _collateralAssets = _collateral();
uint256 _depositedAssets = _totalSupply();

if (_shares == 0 || _depositedAssets == 0) {
return _shares;
}

/**
* @notice Convert `_shares` to `_assets`.
* @param _shares Amount of shares to convert
* @param _exchangeRate Exchange rate used to convert `_shares`
* @param _rounding Rounding mode (i.e. down or up)
* @return uint256 Amount of assets represented by the shares
*/
function _convertToAssets(
uint256 _shares,
uint256 _exchangeRate,
Math.Rounding _rounding
) internal view returns (uint256) {
return
(_shares == 0 || _exchangeRate == 0)
? _shares
: _shares.mulDiv(_exchangeRate, _assetUnit, _rounding);
_collateralAssets == 0 ? 0 : _shares.mulDiv(_collateralAssets, _depositedAssets, _rounding);
}

/* ============ Deposit Functions ============ */
Expand Down Expand Up @@ -1011,7 +997,16 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
);
}

uint256 _withdrawableAssetsBefore = _yieldVault.maxWithdraw(address(this));

_yieldVault.deposit(_assets, address(this));

uint256 _expectedWithdrawableAssets = _withdrawableAssetsBefore + _assets;
uint256 _withdrawableAssetsAfter = _yieldVault.maxWithdraw(address(this));

if (_withdrawableAssetsAfter < _expectedWithdrawableAssets)
revert YVWithdrawableAssetsLTExpected(_withdrawableAssetsAfter, _expectedWithdrawableAssets);

_mint(_receiver, _shares);

emit Deposit(_caller, _receiver, _assets, _shares);
Expand Down Expand Up @@ -1055,6 +1050,8 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
uint256 _assets,
uint256 _shares
) internal virtual override {
if (_assets == 0) revert WithdrawZeroAssets();

if (_caller != _owner) {
_spendAllowance(_owner, _caller, _shares);
}
Expand All @@ -1070,8 +1067,6 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
_yieldVault.withdraw(_assets, address(this), address(this));
SafeERC20.safeTransfer(IERC20(asset()), _receiver, _assets);

_updateExchangeRate();

emit Withdraw(_caller, _receiver, _owner, _assets, _shares);
}

Expand Down Expand Up @@ -1152,26 +1147,18 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {

/* ============ State Functions ============ */

/// @notice Update exchange rate with the current exchange rate.
function _updateExchangeRate() internal {
_lastRecordedExchangeRate = _currentExchangeRate();
emit RecordedExchangeRate(_lastRecordedExchangeRate);
}

/**
* @notice Creates `_shares` tokens and assigns them to `_receiver`, increasing the total supply.
* @param _receiver Address that will receive the minted shares
* @param _shares Shares to mint
* @dev Emits a {Transfer} event with `from` set to the zero address.
* @dev `_receiver` cannot be the zero address.
* @dev Updates the exchange rate.
*/
function _mint(address _receiver, uint256 _shares) internal virtual override {
if (_shares > maxMint(_receiver))
revert MintMoreThanMax(_receiver, _shares, maxMint(_receiver));

_twabController.mint(_receiver, SafeCast.toUint96(_shares));
_updateExchangeRate();

emit Transfer(address(0), _receiver, _shares);
}
Expand All @@ -1183,7 +1170,6 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
* @dev Emits a {Transfer} event with `to` set to the zero address.
* @dev `_owner` cannot be the zero address.
* @dev `_owner` must have at least `_shares` tokens.
* @dev Updates the exchange rate.
*/
function _burn(address _owner, uint256 _shares) internal virtual override {
_twabController.burn(_owner, SafeCast.toUint96(_shares));
Expand All @@ -1207,45 +1193,37 @@ contract Vault is ERC4626, ERC20Permit, ILiquidationSource, Ownable {
}

/**
* @notice Calculate exchange rate between the amount of assets withdrawable from the YieldVault
* and the amount of shares minted by this Vault.
* @dev The amount of yield generated by the YieldVault is excluded from the calculation,
* so users can only withdraw the amount of underlying assets they deposited.
* Except when the vault is undercollateralized, in this case, any unclaimed yield is included.
* @dev We start with an exchange rate of 1 which is equal to 1 underlying asset unit.
* @return uint256 Exchange rate
* @notice Returns the quantity of withdrawable underlying assets held as collateral by the YieldVault.
* @dev When the Vault is collateralized, Vault shares are minted at a 1:1 ratio based on the user's deposited underlying assets.
* The total supply of shares corresponds directly to the total amount of underlying assets deposited into the YieldVault.
* Users have the ability to withdraw only the quantity of underlying assets they initially deposited,
* without access to any of the accumulated yield within the YieldVault.
* @dev In case of undercollateralization, any remaining collateral within the YieldVault can be withdrawn.
* Withdrawals can be made by users for their corresponding deposit shares.
* @return uint256 Available collateral
*/
function _currentExchangeRate() internal view returns (uint256) {
uint256 _totalSupplyAmount = _totalSupply();
uint256 _depositedAssets = _convertToAssets(
_totalSupplyAmount,
_lastRecordedExchangeRate,
Math.Rounding.Down
);

function _collateral() internal view returns (uint256) {
uint256 _depositedAssets = _totalSupply();
uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this));

// If the Vault is collateralized, yield is excluded from the withdrawable amount
// and users can only withdraw the amount of underlying assets they deposited.
// Otherwise, any unclaimed yield is included and shared proportionally amongst depositors.
if (_withdrawableAssets > _depositedAssets) {
_withdrawableAssets = _depositedAssets;
}

if (_totalSupplyAmount != 0 && _withdrawableAssets != 0) {
return _withdrawableAssets.mulDiv(_assetUnit, _totalSupplyAmount, Math.Rounding.Down);
// If the Vault is collateralized, users can only withdraw the amount of underlying assets they deposited.
if (_withdrawableAssets >= _depositedAssets) {
return _depositedAssets;
}

return _assetUnit;
// Otherwise, any remaining collateral within the YieldVault is available
// and distributed proportionally among depositors.
return _withdrawableAssets;
}

/**
* @notice Check if the Vault is collateralized.
* @dev The vault is collateralized if the exchange rate is greater than or equal to 1 underlying asset unit.
* @dev The vault is collateralized if the total amount of underlying assets currently held by the YieldVault
* is greater than or equal to the total supply of shares minted by the Vault.
* @return bool True if the vault is collateralized, false otherwise
*/
function _isVaultCollateralized() internal view returns (bool) {
return _currentExchangeRate() >= _assetUnit;
return _yieldVault.maxWithdraw(address(this)) >= _totalSupply();
}

/// @notice Require reverting if the vault is under-collateralized.
Expand Down
28 changes: 2 additions & 26 deletions test/contracts/mock/YieldVault.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ contract YieldVault is ERC4626Mock {

constructor(address _asset, string memory _name, string memory _symbol) ERC4626Mock(_asset) {}

function burnAssets(address _account, uint256 _assets) external {
ERC20Mock(asset()).burn(_account, _assets);
}

/**
* We override the virtual shares and assets implementation since this approach captures
* a very small part of the yield being accrued, which offsets by 1 wei
Expand All @@ -26,34 +22,14 @@ contract YieldVault is ERC4626Mock {
Math.Rounding rounding
) internal view virtual override returns (uint256) {
uint256 supply = totalSupply();
return
(assets == 0 || supply == 0)
? _initialConvertToShares(assets, rounding)
: assets.mulDiv(supply, totalAssets(), rounding);
}

function _initialConvertToShares(
uint256 assets,
Math.Rounding /*rounding*/
) internal view virtual returns (uint256 shares) {
return assets;
}

function _initialConvertToAssets(
uint256 shares,
Math.Rounding /*rounding*/
) internal view virtual returns (uint256) {
return shares;
return (assets == 0 || supply == 0) ? assets : assets.mulDiv(supply, totalAssets(), rounding);
}

function _convertToAssets(
uint256 shares,
Math.Rounding rounding
) internal view virtual override returns (uint256) {
uint256 supply = totalSupply();
return
(supply == 0)
? _initialConvertToAssets(shares, rounding)
: shares.mulDiv(totalAssets(), supply, rounding);
return (shares == 0 || supply == 0) ? shares : shares.mulDiv(totalAssets(), supply, rounding);
}
}
19 changes: 17 additions & 2 deletions test/unit/Vault/Deposit.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,22 @@ Feature: Deposit
Scenario: Alice deposits into the Vault
Given Alice owns 0 Vault shares
When Alice deposits type(uint96).max + 1 underlying assets
Then the transaction reverts with the custom error DepositMoreThanMax
Then the transaction reverts with the custom error `DepositMoreThanMax`

Scenario: Alice deposits into the Vault
Given Alice owns 0 Vault shares and YieldVault's maxDeposit function returns type(uint88).max
When Alice deposits type(uint88).max + 1 underlying assets
Then the transaction reverts with the custom error DepositMoreThanMax
Then the transaction reverts with the custom error `DepositMoreThanMax`

Scenario: Alice deposits into the Vault
Given Alice owns 0 Vault shares and the YieldVault's exchange rate has been manipulated
When Alice deposits 1,000 underlying assets
Then the transaction reverts with the custom error `YVWithdrawableAssetsLTExpected`

Scenario: Alice deposits into the Vault
Given Alice owns 0 Vault shares and the Vault is undercollateralized
When Alice deposits 1,000 underlying assets
Then the transaction reverts with the custom error `VaultUnderCollateralized`

# Deposit - Attacks
# Inflation attack
Expand Down Expand Up @@ -121,6 +131,11 @@ Feature: Deposit
When Alice mints 0 shares
Then the transaction reverts with the custom error MintZeroShares

Scenario: Alice mints 1,000 shares from the Vault
Given Alice owns 0 Vault shares and the Vault is undercollateralized
When Alice mints 1,000 shares
Then the transaction reverts with the custom error VaultUnderCollateralized

# Sponsor
Scenario: Alice sponsors the Vault
Given Alice owns 0 Vault shares and has not sponsored the Vault
Expand Down
Loading

0 comments on commit 1017a67

Please sign in to comment.