From 6017b2eb712c5c6ef870f2ce5073361f4f7833e6 Mon Sep 17 00:00:00 2001 From: Paul Vienhage Date: Thu, 11 Nov 2021 18:40:55 +0100 Subject: [PATCH 1/5] Removes the gas reserve which is unused in our deployed versions (#218) * remove gas reserve * remove warning * pause and admin upgrade (#222) --- contracts/ConvergentCurvePool.sol | 52 ++- contracts/YVaultAssetProxy.sol | 323 ++++++------------ contracts/YVaultV4AssetProxy.sol | 61 ---- contracts/factories/ConvergentPoolFactory.sol | 7 +- contracts/test/TestConvergentCurvePool.sol | 5 +- contracts/test/TestYVault.sol | 4 + test/convergentCurvePoolTests.ts | 57 +++- test/ethPoolMainnetTest.ts | 4 - test/helpers/deployer.ts | 10 +- test/usdcPoolMainnetTest.ts | 147 -------- test/userProxyTest.ts | 7 - test/wrappedPositionTest.ts | 241 +++++-------- 12 files changed, 316 insertions(+), 602 deletions(-) delete mode 100644 contracts/YVaultV4AssetProxy.sol diff --git a/contracts/ConvergentCurvePool.sol b/contracts/ConvergentCurvePool.sol index ae64e665..a554e555 100644 --- a/contracts/ConvergentCurvePool.sol +++ b/contracts/ConvergentCurvePool.sol @@ -9,6 +9,9 @@ import "./balancer-core-v2/vault/interfaces/IMinimalSwapInfoPool.sol"; import "./balancer-core-v2/vault/interfaces/IVault.sol"; import "./balancer-core-v2/pools/BalancerPoolToken.sol"; +// SECURITY - A governance address can freeze trading and deposits but has no access to user funds +// and cannot stop withdraws. + contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { using LogExpMath for uint256; using FixedPoint for uint256; @@ -30,7 +33,12 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { // The fees recorded during swaps. These will be 18 point not token decimal encoded uint128 public feesUnderlying; - uint128 public feesBond; + // This typing is a weird mismatch but we want to be able to hold a bool as well + uint120 public feesBond; + // A bool to indicate if the contract is paused, stored with 'fees bond' + bool public paused; + // A mapping of who can pause + mapping(address => bool) public pausers; // Stored records of governance tokens address public immutable governance; // The percent of each trade's implied yield to collect as LP fee @@ -49,6 +57,8 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { // The max percent fee for governance, immutable after compilation uint256 public constant FEE_BOUND = 3e17; + // State saying if the contract is paused + /// @notice This event allows the frontend to track the fees /// @param collectedBase the base asset tokens fees collected in this txn /// @param collectedBond the bond asset tokens fees collected in this txn @@ -74,6 +84,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { /// @param _governance The address which gets minted reward lp /// @param name The balancer pool token name /// @param symbol The balancer pool token symbol + /// @param _pauser An address that can pause trades and deposits constructor( IERC20 _underlying, IERC20 _bond, @@ -84,7 +95,8 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { uint256 _percentFeeGov, address _governance, string memory name, - string memory symbol + string memory symbol, + address _pauser ) BalancerPoolToken(name, symbol) { // Sanity Check require(_expiration - block.timestamp < _unitSeconds); @@ -102,6 +114,9 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { tokens[1] = _underlying; } + // Set that the _pauser can pause + pausers[_pauser] = true; + // Pass in zero addresses for Asset Managers // Note - functions below assume this token order vault.registerTokens(poolId, tokens, new address[](2)); @@ -151,7 +166,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { SwapRequest memory swapRequest, uint256 currentBalanceTokenIn, uint256 currentBalanceTokenOut - ) public override returns (uint256) { + ) public override notPaused() returns (uint256) { // Check that the sender is pool, we change state so must make // this check. require(msg.sender == address(_vault), "Non Vault caller"); @@ -235,6 +250,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { ) external override + notPaused() returns ( uint256[] memory amountsIn, uint256[] memory dueProtocolFeeAmounts @@ -377,6 +393,30 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { data[bondIndex] = _normalize(data[bondIndex], 18, bondDecimals); } + // Permission-ed functions + + /// @notice checks for a pause on trading and depositing functionality + modifier notPaused() { + require(!paused, "Paused"); + _; + } + + /// @notice Allows an authorized address or the owner to pause this contract + /// @param pauseStatus true for paused, false for not paused + /// @dev the caller must be authorized + function pause(bool pauseStatus) external { + require(pausers[msg.sender], "Sender not Authorized"); + paused = pauseStatus; + } + + /// @notice Governance sets someone's pause status + /// @param who The address + /// @param status true for able to pause false for not + function setPauser(address who, bool status) external { + require(msg.sender == governance, "Sender not Owner"); + pausers[who] = status; + } + // Math libraries and internal routing /// @dev Calculates how many tokens should be outputted given an input plus reserve variables @@ -449,7 +489,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { amountIn.sub(amountOut) ); // we record that collected fee from the input bond - feesBond += uint128(impliedYieldFee); + feesBond += uint120(impliedYieldFee); // and return the updated input quote return amountIn.add(impliedYieldFee); } @@ -460,7 +500,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { amountOut.sub(amountIn) ); // we record that fee collected from the bond output - feesBond += uint128(impliedYieldFee); + feesBond += uint120(impliedYieldFee); // and then return the updated output return amountOut.sub(impliedYieldFee); } else { @@ -649,7 +689,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { ); // Store the remaining fees feesUnderlying = uint128(remainingUnderlying); - feesBond = uint128(remainingBond); + feesBond = uint120(remainingBond); // We return the fees which were removed from storage return (usedFeeUnderlying, usedFeeBond); } diff --git a/contracts/YVaultAssetProxy.sol b/contracts/YVaultAssetProxy.sol index 356cd4f7..a564da84 100644 --- a/contracts/YVaultAssetProxy.sol +++ b/contracts/YVaultAssetProxy.sol @@ -1,231 +1,116 @@ // SPDX-License-Identifier: Apache-2.0 -// WARNING: This has been validated for yearn vaults up to version 0.2.11. -// Using this code with any later version can be unsafe. pragma solidity ^0.8.0; import "./interfaces/IERC20.sol"; import "./interfaces/IYearnVault.sol"; import "./WrappedPosition.sol"; +import "./libraries/Authorizable.sol"; + +/// SECURITY - This contract has an owner address which can migrate funds to a new yearn vault [or other contract +/// with compatible interface] as well as pause deposits and withdraws. This means that any deposited funds +/// have the same security as that address. /// @author Element Finance /// @title Yearn Vault v1 Asset Proxy -contract YVaultAssetProxy is WrappedPosition { - IYearnVault public immutable vault; +contract YVaultAssetProxy is WrappedPosition, Authorizable { + // The addresses of the current yearn vault + IYearnVault public vault; + // 18 decimal fractional form of the multiplier which is applied after + // a vault upgrade. 0 when no upgrade has happened + uint88 public conversionRate; + // Bool packed into the same storage slot as vault and conversion rate + bool public paused; uint8 public immutable vaultDecimals; - // This contract allows deposits to a reserve which can - // be used to short circuit the deposit process and save gas - - // The following mapping tracks those non-transferable deposits - mapping(address => uint256) public reserveBalances; - // These variables store the token balances of this contract and - // should be packed by solidity into a single slot. - uint128 public reserveUnderlying; - uint128 public reserveShares; - // This is the total amount of reserve deposits - uint256 public reserveSupply; - /// @notice Constructs this contract and stores needed data /// @param vault_ The yearn v2 vault /// @param _token The underlying token. /// This token should revert in the event of a transfer failure. /// @param _name The name of the token created /// @param _symbol The symbol of the token created + /// @param _governance The address which can upgrade the yearn vault + /// @param _pauser address which can pause this contract constructor( address vault_, IERC20 _token, string memory _name, - string memory _symbol - ) WrappedPosition(_token, _name, _symbol) { + string memory _symbol, + address _governance, + address _pauser + ) WrappedPosition(_token, _name, _symbol) Authorizable() { + // Authorize the pauser + _authorize(_pauser); + // set the owner + setOwner(_governance); + // Set the vault vault = IYearnVault(vault_); + // Approve the vault so it can pull tokens from this address _token.approve(vault_, type(uint256).max); + // Load the decimals and set them as an immutable uint8 localVaultDecimals = IERC20(vault_).decimals(); vaultDecimals = localVaultDecimals; require( uint8(_token.decimals()) == localVaultDecimals, "Inconsistent decimals" ); - // We check that this is a compatible yearn version - _versionCheck(IYearnVault(vault_)); - } - - /// @notice An override-able version checking function, reverts if the vault has the wrong version - /// @param _vault The yearn vault address - /// @dev This function can be overridden by an inheriting upgrade contract - function _versionCheck(IYearnVault _vault) internal view virtual { - string memory apiVersion = _vault.apiVersion(); - require( - _stringEq(apiVersion, "0.3.0") || - _stringEq(apiVersion, "0.3.1") || - _stringEq(apiVersion, "0.3.2") || - _stringEq(apiVersion, "0.3.3") || - _stringEq(apiVersion, "0.3.4") || - _stringEq(apiVersion, "0.3.5"), - "Unsupported Version" - ); - } - - /// @notice checks if two strings are equal - /// @param s1 string one - /// @param s2 string two - /// @return bool whether they are equal - function _stringEq(string memory s1, string memory s2) - internal - pure - returns (bool) - { - bytes32 h1 = keccak256(abi.encodePacked(s1)); - bytes32 h2 = keccak256(abi.encodePacked(s2)); - return (h1 == h2); } - /// @notice This function allows a user to deposit to the reserve - /// Note - there's no incentive to do so. You could earn some - /// interest but less interest than yearn. All deposits use - /// the underlying token. - /// @param _amount The amount of underlying to deposit - function reserveDeposit(uint256 _amount) external { - // Transfer from user, note variable 'token' is the immutable - // inherited from the abstract WrappedPosition contract. - token.transferFrom(msg.sender, address(this), _amount); - // Load the reserves - (uint256 localUnderlying, uint256 localShares) = _getReserves(); - // Calculate the total reserve value - uint256 totalValue = localUnderlying; - totalValue += _yearnDepositConverter(localShares, true); - // If this is the first deposit we need different logic - uint256 localReserveSupply = reserveSupply; - uint256 mintAmount; - if (localReserveSupply == 0) { - // If this is the first mint the tokens are exactly the supplied underlying - mintAmount = _amount; - } else { - // Otherwise we mint the proportion that this increases the value held by this contract - mintAmount = (localReserveSupply * _amount) / totalValue; - } - - // This hack means that the contract will never have zero balance of underlying - // which levels the gas expenditure of the transfer to this contract. Permanently locks - // the smallest possible unit of the underlying. - if (localUnderlying == 0 && localShares == 0) { - _amount -= 1; - } - // Set the reserves that this contract has more underlying - _setReserves(localUnderlying + _amount, localShares); - // Note that the sender has deposited and increase reserveSupply - reserveBalances[msg.sender] += mintAmount; - reserveSupply = localReserveSupply + mintAmount; - } - - /// @notice This function allows a holder of reserve balance to withdraw their share - /// @param _amount The number of reserve shares to withdraw - function reserveWithdraw(uint256 _amount) external { - // Remove 'amount' from the balances of the sender. Because this is 8.0 it will revert on underflow - reserveBalances[msg.sender] -= _amount; - // We load the reserves - (uint256 localUnderlying, uint256 localShares) = _getReserves(); - uint256 localReserveSupply = reserveSupply; - // Then we calculate the proportion of the shares to redeem - uint256 userShares = (localShares * _amount) / localReserveSupply; - // First we withdraw the proportion of shares tokens belonging to the caller - uint256 freedUnderlying = vault.withdraw(userShares, address(this), 0); - // We calculate the amount of underlying to send - uint256 userUnderlying = (localUnderlying * _amount) / - localReserveSupply; - - // We then store the updated reserve amounts - _setReserves( - localUnderlying - userUnderlying, - localShares - userShares - ); - // We note a reduction in local supply - reserveSupply = localReserveSupply - _amount; - - // We send the redemption underlying to the caller - // Note 'token' is an immutable from shares - token.transfer(msg.sender, freedUnderlying + userUnderlying); + /// @notice Checks that the contract has not been paused + modifier notPaused() { + require(!paused, "Paused"); + _; } /// @notice Makes the actual deposit into the yearn vault - /// Tries to use the local balances before depositing /// @return Tuple (the shares minted, amount underlying used) - function _deposit() internal override returns (uint256, uint256) { - //Load reserves - (uint256 localUnderlying, uint256 localShares) = _getReserves(); + function _deposit() + internal + override + notPaused() + returns (uint256, uint256) + { // Get the amount deposited - uint256 amount = token.balanceOf(address(this)) - localUnderlying; - // fixing for the fact there's an extra underlying - if (localUnderlying != 0 || localShares != 0) { - amount -= 1; - } - // Calculate the amount of shares the amount deposited is worth - uint256 neededShares = _yearnDepositConverter(amount, false); + uint256 amount = token.balanceOf(address(this)); - // If we have enough in local reserves we don't call out for deposits - if (localShares > neededShares) { - // We set the reserves - _setReserves(localUnderlying + amount, localShares - neededShares); - // And then we short circuit execution and return - return (neededShares, amount); - } // Deposit and get the shares that were minted to this - uint256 shares = vault.deposit(localUnderlying + amount, address(this)); - - // calculate the user share - uint256 userShare = (amount * shares) / (localUnderlying + amount); + uint256 shares = vault.deposit(amount, address(this)); + + // If we have migrated our shares are no longer 1 - 1 with the vault shares + if (conversionRate != 0) { + // conversionRate is the fraction of yearnSharePrice1/yearnSharePrices2 at time of migration + // and so this multiplication will convert between yearn shares in the new vault and + // those in the old vault + shares = (shares * conversionRate) / 1e18; + } - // We set the reserves - _setReserves(0, localShares + shares - userShare); // Return the amount of shares the user has produced, and the amount used for it. - return (userShare, amount); + return (shares, amount); } - /// @notice Withdraw the number of shares and will short circuit if it can - /// @param _shares The number of shares to withdraw + /// @notice Withdraw the number of shares + /// @param _shares The number of wrapped position shares to withdraw /// @param _destination The address to send the output funds - /// @param _underlyingPerShare The possibly precomputed underlying per share + // @param _underlyingPerShare The possibly precomputed underlying per share + /// @return returns the amount of funds freed by doing a yearn withdraw function _withdraw( uint256 _shares, address _destination, - uint256 _underlyingPerShare - ) internal override returns (uint256) { - // If we do not have it we load the price per share - if (_underlyingPerShare == 0) { - _underlyingPerShare = _pricePerShare(); - } - // We load the reserves - (uint256 localUnderlying, uint256 localShares) = _getReserves(); - // Calculate the amount of shares the amount deposited is worth - uint256 needed = (_shares * _pricePerShare()) / (10**vaultDecimals); - // If we have enough underlying we don't have to actually withdraw - if (needed < localUnderlying) { - // We set the reserves to be the new reserves - _setReserves(localUnderlying - needed, localShares + _shares); - // Then transfer needed underlying to the destination - // 'token' is an immutable in WrappedPosition - token.transfer(_destination, needed); - // Short circuit and return - return (needed); + uint256 + ) internal override notPaused() returns (uint256) { + // If the conversion rate is non-zero we have upgraded and so our wrapped shares are + // not one to one with the original shares. + if (conversionRate != 0) { + // Then since conversion rate is yearnSharePrice1/yearnSharePrices2 we divide the + // wrapped position shares by it because they are equivalent to the first yearn vault shares + _shares = (_shares * 1e18) / conversionRate; } - // If we don't have enough local reserves we do the actual withdraw // Withdraws shares from the vault. Max loss is set at 100% as // the minimum output value is enforced by the calling // function in the WrappedPosition contract. - uint256 amountReceived = vault.withdraw( - _shares + localShares, - address(this), - 10000 - ); - - // calculate the user share - uint256 userShare = (_shares * amountReceived) / - (localShares + _shares); + uint256 amountReceived = vault.withdraw(_shares, _destination, 10000); - _setReserves(localUnderlying + amountReceived - userShare, 0); - // Transfer the underlying to the destination 'token' is an immutable in WrappedPosition - token.transfer(_destination, userShare); // Return the amount of underlying - return userShare; + return amountReceived; } /// @notice Get the underlying amount of tokens per shares given @@ -237,6 +122,11 @@ contract YVaultAssetProxy is WrappedPosition { override returns (uint256) { + // We may have to convert before using the vault price per share + if (conversionRate != 0) { + // Imitate the _withdraw logic and convert this amount to yearn vault2 shares + _amount = (_amount * 1e18) / conversionRate; + } return (_amount * _pricePerShare()) / (10**vaultDecimals); } @@ -252,49 +142,48 @@ contract YVaultAssetProxy is WrappedPosition { token.approve(address(vault), type(uint256).max); } - /// @notice Helper to get the reserves with one sload - /// @return Tuple (reserve underlying, reserve shares) - function _getReserves() internal view returns (uint256, uint256) { - return (uint256(reserveUnderlying), uint256(reserveShares)); - } - - /// @notice Helper to set reserves using one sstore - /// @param _newReserveUnderlying The new reserve of underlying - /// @param _newReserveShares The new reserve of wrapped position shares - function _setReserves( - uint256 _newReserveUnderlying, - uint256 _newReserveShares - ) internal { - reserveUnderlying = uint128(_newReserveUnderlying); - reserveShares = uint128(_newReserveShares); + /// @notice Allows an authorized address or the owner to pause this contract + /// @param pauseStatus true for paused, false for not paused + /// @dev the caller must be authorized + function pause(bool pauseStatus) external onlyAuthorized() { + paused = pauseStatus; } - /// @notice Converts an input of shares to it's output of underlying or an input - /// of underlying to an output of shares, using yearn 's deposit pricing - /// @param amount the amount of input, shares if 'sharesIn == true' underlying if not - /// @param sharesIn true to convert from yearn shares to underlying, false to convert from - /// underlying to yearn shares - /// @dev WARNING - In yearn 0.3.1 - 0.3.5 this is an exact match for deposit logic - /// but not withdraw logic in versions 0.3.2-0.3.5. In versions 0.4.0+ - /// it is not a match for yearn deposit ratios. - /// @return The converted output of either underlying or yearn shares - function _yearnDepositConverter(uint256 amount, bool sharesIn) - internal - view - virtual - returns (uint256) + /// @notice Function to transition between two yearn vaults + /// @param newVault The address of the new vault + /// @param minOutputShares The min of the new yearn vault's shares the wp will receive + /// @dev WARNING - This function has the capacity to steal all user funds from this + /// contract and so it should be ensured that the owner is a high quorum + /// governance vote through the time lock. + function transition(IYearnVault newVault, uint256 minOutputShares) + external + onlyOwner { - // Load the yearn total supply and assets - uint256 yearnTotalSupply = vault.totalSupply(); - uint256 yearnTotalAssets = vault.totalAssets(); - // If we are converted shares to underlying - if (sharesIn) { - // then we get the fraction of yearn shares this is and multiply by assets - return (yearnTotalAssets * amount) / yearnTotalSupply; - } else { - // otherwise we figure out the faction of yearn assets this is and see how - // many assets we get out. - return (yearnTotalSupply * amount) / yearnTotalAssets; - } + // Load the current vault's price per share + uint256 currentPricePerShare = _pricePerShare(); + // Load the new vault's price per share + uint256 newPricePerShare = newVault.pricePerShare(); + // Load the current conversion rate or set it to 1 + uint256 newConversionRate = conversionRate == 0 ? 1e18 : conversionRate; + // Calculate the new conversion rate, note by multiplying by the old + // conversion rate here we implicitly support more than 1 upgrade + newConversionRate = + (newConversionRate * newPricePerShare) / + currentPricePerShare; + // We now withdraw from the old yearn vault using max shares + // Note - Vaults should be checked in the future that they still have this behavior + vault.withdraw(type(uint256).max, address(this), 10000); + // Approve the new vault + token.approve(address(newVault), type(uint256).max); + // Then we deposit into the new vault + uint256 currentBalance = token.balanceOf(address(this)); + uint256 outputShares = newVault.deposit(currentBalance, address(this)); + // We enforce a min output + require(outputShares >= minOutputShares, "Not enough output"); + // Change the stored variables + vault = newVault; + // because of the truncation yearn vaults can't have a larger diff than ~ billion + // times larger + conversionRate = uint88(newConversionRate); } } diff --git a/contracts/YVaultV4AssetProxy.sol b/contracts/YVaultV4AssetProxy.sol deleted file mode 100644 index 7bf18432..00000000 --- a/contracts/YVaultV4AssetProxy.sol +++ /dev/null @@ -1,61 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// WARNING: This has been validated for yearn vaults version 4.2, do not use for lower or higher -// versions without review -pragma solidity ^0.8.0; - -import "./interfaces/IERC20.sol"; -import "./interfaces/IYearnVault.sol"; -import "./YVaultAssetProxy.sol"; - -/// @author Element Finance -/// @title Yearn Vault Asset Proxy -contract YVaultV4AssetProxy is YVaultAssetProxy { - /// @notice Constructs this contract by calling into the super constructor - /// @param vault_ The yearn v2 vault, must be version 0.4.2 - /// @param _token The underlying token. - /// This token should revert in the event of a transfer failure. - /// @param _name The name of the token created - /// @param _symbol The symbol of the token created - constructor( - address vault_, - IERC20 _token, - string memory _name, - string memory _symbol - ) YVaultAssetProxy(vault_, _token, _name, _symbol) {} - - /// @notice Overrides the version checking to check for 0.4.2 instead - /// @param _vault The yearn vault address - /// @dev This function can be overridden by an inheriting upgrade contract - function _versionCheck(IYearnVault _vault) internal view override { - string memory apiVersion = _vault.apiVersion(); - require( - _stringEq(apiVersion, "0.4.2") || _stringEq(apiVersion, "0.4.3"), - "Unsupported Version" - ); - } - - /// @notice Converts an input of shares to it's output of underlying or an input - /// of underlying to an output of shares, using yearn 's deposit pricing - /// @param amount the amount of input, shares if 'sharesIn == true' underlying if not - /// @param sharesIn true to convert from yearn shares to underlying, false to convert from - /// underlying to yearn shares - /// @dev WARNING - Fails for 0.3.2-0.3.5, please only use with 0.4.2 - /// @return The converted output of either underlying or yearn shares - function _yearnDepositConverter(uint256 amount, bool sharesIn) - internal - view - override - returns (uint256) - { - // Load the yearn price per share - uint256 pricePerShare = vault.pricePerShare(); - // If we are converted shares to underlying - if (sharesIn) { - // If the input is shares we multiply by the price per share - return (pricePerShare * amount) / 10**vaultDecimals; - } else { - // If the input is in underlying we divide by price per share - return (amount * 10**vaultDecimals) / (pricePerShare + 1); - } - } -} diff --git a/contracts/factories/ConvergentPoolFactory.sol b/contracts/factories/ConvergentPoolFactory.sol index 76b69946..d0d06453 100644 --- a/contracts/factories/ConvergentPoolFactory.sol +++ b/contracts/factories/ConvergentPoolFactory.sol @@ -41,6 +41,7 @@ contract ConvergentPoolFactory is BasePoolFactory, Authorizable { /// @param _percentFee The fee percent of each trades implied yield paid to gov. /// @param _name The name of the balancer v2 lp token for this pool /// @param _symbol The symbol of the balancer v2 lp token for this pool + /// @param _pauser An address with the power to stop trading and deposits /// @return The new pool address function create( address _underlying, @@ -49,7 +50,8 @@ contract ConvergentPoolFactory is BasePoolFactory, Authorizable { uint256 _unitSeconds, uint256 _percentFee, string memory _name, - string memory _symbol + string memory _symbol, + address _pauser ) external returns (address) { address pool = address( new ConvergentCurvePool( @@ -62,7 +64,8 @@ contract ConvergentPoolFactory is BasePoolFactory, Authorizable { percentFeeGov, governance, _name, - _symbol + _symbol, + _pauser ) ); // Register the pool with the vault diff --git a/contracts/test/TestConvergentCurvePool.sol b/contracts/test/TestConvergentCurvePool.sol index 5cf052b2..7eb42bf4 100644 --- a/contracts/test/TestConvergentCurvePool.sol +++ b/contracts/test/TestConvergentCurvePool.sol @@ -29,7 +29,8 @@ contract TestConvergentCurvePool is ConvergentCurvePool { _percentFee, _governance, name, - symbol + symbol, + _governance ) {} // solhint-disable-line no-empty-blocks @@ -103,7 +104,7 @@ contract TestConvergentCurvePool is ConvergentCurvePool { } // Allows tests to specify fees without making trades - function setFees(uint128 amountUnderlying, uint128 amountBond) public { + function setFees(uint128 amountUnderlying, uint120 amountBond) public { feesUnderlying = amountUnderlying; feesBond = amountBond; } diff --git a/contracts/test/TestYVault.sol b/contracts/test/TestYVault.sol index 1cfef2ac..f8aadf43 100644 --- a/contracts/test/TestYVault.sol +++ b/contracts/test/TestYVault.sol @@ -43,6 +43,10 @@ contract TestYVault is ERC20PermitWithSupply { address destination, uint256 ) external returns (uint256) { + // Yearn supports this + if (_shares == type(uint256).max) { + _shares = balanceOf[msg.sender]; + } uint256 _amount = (_shares * pricePerShare()) / (10**decimals); _burn(msg.sender, _shares); IERC20(token).transfer(destination, _amount); diff --git a/test/convergentCurvePoolTests.ts b/test/convergentCurvePoolTests.ts index cd57c270..debe7c80 100644 --- a/test/convergentCurvePoolTests.ts +++ b/test/convergentCurvePoolTests.ts @@ -817,7 +817,8 @@ describe("ConvergentCurvePool", function () { SECONDS_IN_YEAR, 1, "fake pool", - "FP" + "FP", + elementSigner.address ); }); it("Allows changing fees", async () => { @@ -838,4 +839,58 @@ describe("ConvergentCurvePool", function () { await expect(tx).to.be.revertedWith("Sender not owner"); }); }); + + describe("Pause function", async () => { + beforeEach(async () => { + createSnapshot(provider); + }); + afterEach(async () => { + restoreSnapshot(provider); + }); + it("Only lets gov set pause status", async () => { + await poolContract.setPauser(balancerSigner.address, true); + const tx = poolContract + .connect(balancerSigner) + .setPauser(balancerSigner.address, true); + await expect(tx).to.be.revertedWith("Sender not Owner"); + }); + it("Only let's pausers pause", async () => { + await poolContract.pause(true); + const tx = poolContract.connect(balancerSigner).pause(false); + await expect(tx).to.be.revertedWith("Sender not Authorized"); + }); + it("Blocks trades and deposits on a paused pool", async () => { + await poolContract.pause(true); + + let tx = poolContract.onJoinPool( + "0xb6749d30a0b09b310151e2cd2db8f72dd34aab4bbc60cf3e8dbca13b4d9369ad", + fakeAddress, + tokenSigner.address, + // Pool reserves are [100, 50] + [0, 0], + 0, + ethers.utils.parseEther("0.1"), + "0x" + ); + await expect(tx).to.be.revertedWith("Paused"); + tx = poolContract.onSwap( + { + tokenIn: baseAssetContract.address, + tokenOut: bondAssetContract.address, + amount: ethers.utils.parseUnits("100", BASE_DECIMALS), + kind: inForOutType, + // Misc data + poolId: + "0xf4cc12715b126dabd383d98cfad15b0b6c3814ad57c5b9e22d941b5fcd3e4e43", + lastChangeBlock: BigNumber.from(0), + from: fakeAddress, + to: fakeAddress, + userData: "0x", + }, + reserveUnderlying, + reserveBond + ); + await expect(tx).to.be.revertedWith("Paused"); + }); + }); }); diff --git a/test/ethPoolMainnetTest.ts b/test/ethPoolMainnetTest.ts index 159af0c6..59084b73 100644 --- a/test/ethPoolMainnetTest.ts +++ b/test/ethPoolMainnetTest.ts @@ -61,10 +61,6 @@ describe("ETHPool-Mainnet", () => { await fixture.weth .connect(users[3].user) .approve(fixture.position.address, utils.parseEther("90000")); - // Initialize a reserve - await fixture.position - .connect(users[3].user) - .reserveDeposit(utils.parseEther("30000")); }); after(async () => { // revert back to initial state after all tests pass diff --git a/test/helpers/deployer.ts b/test/helpers/deployer.ts index a69175c7..92e16457 100644 --- a/test/helpers/deployer.ts +++ b/test/helpers/deployer.ts @@ -118,7 +118,15 @@ const deployYasset = async ( symbol: string ) => { const yVaultDeployer = new YVaultAssetProxy__factory(signer); - return await yVaultDeployer.deploy(yUnderlying, underlying, name, symbol); + const signerAddress = await signer.getAddress(); + return await yVaultDeployer.deploy( + yUnderlying, + underlying, + name, + symbol, + signerAddress, + signerAddress + ); }; const deployInterestTokenFactory = async (signer: Signer) => { diff --git a/test/usdcPoolMainnetTest.ts b/test/usdcPoolMainnetTest.ts index 901130ad..bb8eca9d 100644 --- a/test/usdcPoolMainnetTest.ts +++ b/test/usdcPoolMainnetTest.ts @@ -208,151 +208,4 @@ describe("USDCPool-Mainnet", () => { const yearnTotalAssets = await fixture.yusdc.totalAssets(); return yearnTotalAssets.mul(amount).div(yearnTotalSupply); } - - describe("Funded Reserve Deposit/Withdraw", () => { - it("should correctly handle deposits and withdrawals", async () => { - impersonate(usdcWhaleAddress); - const usdcWhale = ethers.provider.getSigner(usdcWhaleAddress); - // A user funds the reserves to enable gas efficient actions - const initialReserve = 3e11; - await fixture.usdc - .connect(usdcWhale) - .approve(fixture.position.address, 4e11); - await fixture.position - .connect(usdcWhale) - .reserveDeposit(initialReserve + 1); - let deposit = await yearnDepositSim(fixture, initialReserve); - let pricePerFullShare = await fixture.yusdc.pricePerShare(); - - // Now we try some deposits - // Note we use less here because the second calc has some rounding error - // In the first trade the reserve has no wrapped position shares - await fixture.position - .connect(users[1].user) - .deposit(users[1].address, 1e11); - let userBalance = await fixture.position.balanceOf(users[1].address); - let shareAmount = BigNumber.from(1e11).mul(1e6).div(pricePerFullShare); - expect(userBalance).to.be.at.least(subError(shareAmount)); - const shareReserve = await fixture.position.reserveShares(); - expect(shareReserve).to.be.eq(deposit); - expect(await fixture.position.reserveUnderlying()).to.be.eq(0); - - // The second is fully fillable from the reserve's wrapped position shares - deposit = await yearnDepositSim(fixture, 2e11); - await fixture.position - .connect(users[2].user) - .deposit(users[2].address, 2e11); - userBalance = await fixture.position.balanceOf(users[2].address); - shareAmount = BigNumber.from(2e11).mul(1e6).div(pricePerFullShare); - expect(userBalance).to.be.at.least(subError(shareAmount)); - expect(await fixture.position.reserveShares()).to.be.eq( - shareReserve.sub(deposit) - ); - expect(await fixture.position.reserveUnderlying()).to.be.eq(2e11); - - // The third consumes the remaining requiring switchover - deposit = await yearnDepositSim(fixture, initialReserve + 1); - await fixture.usdc.connect(users[4].user).transfer(users[1].address, 1); - // Note we have to add 1 unit to this transfer to cause the right behavior - await fixture.position - .connect(users[1].user) - .deposit(users[1].address, 1e11 + 1); - userBalance = await fixture.position.balanceOf(users[1].address); - shareAmount = BigNumber.from(2e11).mul(1e6).div(pricePerFullShare); - expect(userBalance).to.be.at.least(subError(shareAmount)); - expect(await fixture.position.reserveShares()).to.be.at.least( - subError(deposit) - ); - expect(await fixture.position.reserveUnderlying()).to.be.eq(0); - - // Final deposit should be preformed without the reserve - await fixture.position - .connect(users[3].user) - .deposit(users[3].address, 6e11); - userBalance = await fixture.position.balanceOf(users[3].address); - shareAmount = BigNumber.from(6e11).mul(1e6).div(pricePerFullShare); - expect(userBalance).to.be.at.least(subError(shareAmount)); - expect(await fixture.position.reserveShares()).to.be.at.least( - subError(deposit) - ); - expect(await fixture.position.reserveUnderlying()).to.be.eq(0); - - // Test withdraws - const toWithdraw = ethers.BigNumber.from("1000000"); // 1 usdc - const user1Balance = await fixture.position.balanceOf(users[1].address); - pricePerFullShare = await fixture.yusdc.pricePerShare(); - const withdrawUsdc = toWithdraw - .mul(pricePerFullShare) - .div(ethers.utils.parseUnits("1", 6)); - - let usdcBalance = await yearnWithdrawSim(fixture, deposit); - await fixture.position - .connect(users[1].user) - .withdraw(users[1].address, toWithdraw, 0); - expect(await fixture.position.balanceOf(users[1].address)).to.equal( - user1Balance.sub(toWithdraw) - ); - expect(await fixture.usdc.balanceOf(users[1].address)).to.equal( - withdrawUsdc - ); - expect(await fixture.position.reserveShares()).to.be.eq(0); - // The extra unit is from rounding error in the contract's favor - expect(await fixture.position.reserveUnderlying()).to.be.at.least( - subError(usdcBalance) - ); - - const shareBalanceU1 = await fixture.position.balanceOf(users[1].address); - let newWithdraw = await yearnWithdrawSim(fixture, shareBalanceU1); - await fixture.position - .connect(users[1].user) - .withdraw(users[1].address, shareBalanceU1, 0); - expect(await fixture.position.balanceOf(users[1].address)).to.equal(0); - expect(await fixture.position.reserveShares()).to.be.eq(shareBalanceU1); - expect(await fixture.position.reserveUnderlying()).to.be.at.least( - usdcBalance.sub(newWithdraw) - ); - // We record the current share balance - usdcBalance = usdcBalance.add(1).sub(newWithdraw); - - // This withdraw requires resetting the reserves - const shareBalanceU2 = await fixture.position.balanceOf(users[2].address); - newWithdraw = await yearnWithdrawSim(fixture, shareBalanceU2); - await fixture.position - .connect(users[2].user) - .withdraw(users[2].address, shareBalanceU2, 0); - expect(await fixture.position.balanceOf(users[2].address)).to.equal(0); - expect(await fixture.position.reserveShares()).to.be.eq(0); - expect(await fixture.position.reserveUnderlying()).to.be.at.least( - initialReserve + 1 - ); - - // This withdraw cannot use the reserves so doesn't change them - const shareBalanceU3 = await fixture.position.balanceOf(users[3].address); - await fixture.position - .connect(users[3].user) - .withdraw(users[3].address, shareBalanceU3, 0); - expect(await fixture.position.balanceOf(users[3].address)).to.equal(0); - expect(await fixture.position.reserveShares()).to.be.eq(0); - expect(await fixture.position.reserveUnderlying()).to.be.at.least( - initialReserve + 1 - ); - - /* At this point: - * deposited held - * User 1: 0 USDC | 50,000 USDC - * user 2: 0 USDC | 20,000 USDC - * User 3: 0 USDC | 30,000 USDC - */ - - const finalWethBalanceU1 = await fixture.usdc.balanceOf(users[1].address); - const finalWethBalanceU2 = await fixture.usdc.balanceOf(users[2].address); - const finalWethBalanceU3 = await fixture.usdc.balanceOf(users[3].address); - expect( - finalWethBalanceU1 - .add(finalWethBalanceU2) - .add(finalWethBalanceU3) - .add(ethers.BigNumber.from("5")) - ).to.be.at.least(subError(BigNumber.from("1000000000000"))); - }); - }); }); diff --git a/test/userProxyTest.ts b/test/userProxyTest.ts index 7abdd04c..c0d177da 100644 --- a/test/userProxyTest.ts +++ b/test/userProxyTest.ts @@ -54,13 +54,6 @@ describe("UserProxyTests", function () { await usdcFixture.usdc.connect(usdcWhale).transfer(signers[0].address, 100); await usdcFixture.usdc.approve(usdcFixture.yusdc.address, 100); await usdcFixture.yusdc.deposit(100, signers[0].address); - - // Make a gas reserve deposit to test that logic - const twohundred = ethers.utils.parseUnits("200", 6); - await usdcFixture.usdc - .connect(usdcWhale) - .approve(usdcFixture.position.address, twohundred.mul(2)); - await usdcFixture.position.connect(usdcWhale).reserveDeposit(twohundred); }); after(async () => { // revert back to initial state after all tests pass diff --git a/test/wrappedPositionTest.ts b/test/wrappedPositionTest.ts index d65cc847..e677bf6f 100644 --- a/test/wrappedPositionTest.ts +++ b/test/wrappedPositionTest.ts @@ -5,8 +5,8 @@ import { ethers, waffle } from "hardhat"; import { FixtureInterface, loadFixture } from "./helpers/deployer"; import { createSnapshot, restoreSnapshot } from "./helpers/snapshots"; -import { TestYVaultV4 } from "typechain/TestYVaultV4"; -import { YVaultV4AssetProxy } from "typechain/YVaultV4AssetProxy"; +import { TestYVault } from "../typechain/TestYVault"; +import { TestYVault__factory } from "../typechain/factories/TestYVault__factory"; const { provider } = waffle; @@ -48,29 +48,6 @@ describe("Wrapped Position", () => { // revert back to initial state after all tests pass await restoreSnapshot(provider); }); - describe("Version locked deployments", () => { - it("Wont deploy YAssetProxy with a v4 yearn vault", async () => { - // Deploy a mock yearn v4 - const mockYearnV4Factory = await ethers.getContractFactory( - "TestYVaultV4" - ); - const yVaultV4 = await mockYearnV4Factory.deploy( - fixture.usdc.address, - await fixture.usdc.decimals() - ); - // Attempt to deploy a non v4 wp on it - const yearnWPFactory = await ethers.getContractFactory( - "YVaultAssetProxy" - ); - const tx = yearnWPFactory.deploy( - yVaultV4.address, - fixture.usdc.address, - "mock yearn", - "my" - ); - await expect(tx).to.be.revertedWith("Unsupported Version"); - }); - }); describe("balanceOfUnderlying", () => { beforeEach(async () => { await createSnapshot(provider); @@ -176,156 +153,112 @@ describe("Wrapped Position", () => { expect(totalUsdcBalance).to.equal(ethers.BigNumber.from("19000000")); }); }); - // These tests are of the reserve mechanic - describe("Reserve deposit and withdraw", async () => { + describe("Yearn migration", async () => { + let newVault: TestYVault; + + before(async () => { + // Deploy a new version + const yearnDeployer = new TestYVault__factory(users[0].user); + newVault = await yearnDeployer.deploy( + fixture.usdc.address, + await fixture.usdc.decimals() + ); + // Deposit into it and set the ratio + await fixture.usdc.mint(users[0].address, 100); + await fixture.usdc.approve(newVault.address, 100); + await newVault.deposit(100, users[0].address); + // This ensures theirs a difference between the price per shares + await newVault.updateShares(); + await newVault.updateShares(); + }); + // tests are independent beforeEach(async () => { await createSnapshot(provider); }); afterEach(async () => { await restoreSnapshot(provider); }); - it("Successfully deposits", async () => { - // Create some underlying - await fixture.usdc.mint(users[0].address, 100e6); - // Approve the wrapped position reserve - await fixture.usdc.approve(fixture.position.address, 100e6); - // deposit to the reserves - await fixture.position.reserveDeposit(10e6); - expect(await fixture.position.reserveUnderlying()).to.be.eq(10e6 - 1); - expect(await fixture.position.reserveShares()).to.be.eq(0); - expect(await fixture.position.reserveBalances(users[0].address)).to.be.eq( - 10e6 - ); - expect(await fixture.position.reserveSupply()).to.be.eq(10e6); - // We make a small deposit to change the reserve - await fixture.position - .connect(users[1].user) - .deposit(users[1].address, 1e6); - // NOTE - THIS CONSTANT DEPENDS ON THE PREVIOUS TEST SETTING THE DEPOSIT RATE - expect(await fixture.position.balanceOf(users[1].address)).to.equal( - 909090 - ); - // We then make a new reserve deposit - await fixture.position.connect(users[2].user).reserveDeposit(1e6); - expect(await fixture.position.reserveUnderlying()).to.be.eq(1e6); - expect(await fixture.position.reserveShares()).to.be.eq(9090909); - expect(await fixture.position.reserveBalances(users[2].address)).to.be.eq( - 1e6 + + it("Allows governance to upgrade", async () => { + await fixture.position.transition(newVault.address, 0); + const conversionRate = await fixture.position.conversionRate(); + // Magic hex is 1.1 in 18 point fixed + expect(conversionRate).to.be.eq(BigNumber.from("0xF43FC2C04EE0000")); + }); + + it("Blocks non governance upgrades", async () => { + const tx = fixture.position + .connect(users[2].user) + .transition(newVault.address, 0); + await expect(tx).to.be.revertedWith("Sender not owner"); + }); + + it("Blocks withdraw which does not product enough tokens", async () => { + const tx = fixture.position.transition( + newVault.address, + ethers.constants.MaxUint256 ); - expect(await fixture.position.reserveSupply()).to.be.eq(11e6); + await expect(tx).to.be.revertedWith("Not enough output"); }); - it("Successfully withdraws", async () => { - // Create some underlying - await fixture.usdc.mint(users[0].address, 10e6); - // Approve the position reserve - await fixture.usdc.approve(fixture.position.address, 100e6); - // deposit to the reserve - await fixture.position.reserveDeposit(10e6); - const balanceStart = await fixture.usdc.balanceOf(users[0].address); - // We do a trial withdraw - await fixture.position.reserveWithdraw(1e6); - let currentBalance = await fixture.usdc.balanceOf(users[0].address); - // Note - this is slightly less because underlying reserves are missing - // exactly one unit of usdc - expect(currentBalance).to.be.eq(balanceStart.add(999999)); - // We now check that reserve withdraws with shares present work properly - // Make a trade which requires local shares - await fixture.position - .connect(users[1].user) - .deposit(users[1].address, 1e6); - expect(await fixture.position.reserveShares()).to.be.eq(8181819); - // We now make a withdraw and check that the right amount is produced - await fixture.position.reserveWithdraw(1e6); - currentBalance = await fixture.usdc.balanceOf(users[0].address); - expect(currentBalance).to.be.eq(balanceStart.add(999999).add(1e6)); - expect(await fixture.position.reserveShares()).to.be.eq(7272728); + it("Makes consistent deposits", async () => { + // We check that a deposit before an upgrade gets the same amount of shares as one after + await fixture.position.deposit(users[0].address, 1e6); + const beforeBalance = await fixture.position.balanceOf(users[0].address); + await fixture.position.transition(newVault.address, 0); + await fixture.position.deposit(users[1].address, 1e6); + const afterBalance = await fixture.position.balanceOf(users[1].address); + // There are very small rounding errors leading to -1 + expect(beforeBalance.sub(1)).to.be.eq(afterBalance); }); - }); - // We test explicity only the functionality changes in V4 not the whole - // contract - describe("Wrapped Yearn Vault v4 upgrade", async () => { - let vaultV4: TestYVaultV4; - let wpV4: YVaultV4AssetProxy; - before(async () => { - // Deploy a mock yearn v4 - const mockYearnV4Factory = await ethers.getContractFactory( - "TestYVaultV4" + // We check that after a transition you can still withdraw the same amount + it("Makes consistent withdraws", async () => { + // NOTE - Because of a rounding error bug the conversion rate mechanic can cause withdraw + // failure for the last withdraw. we fix here by having a second deposit + await fixture.position.deposit(users[1].address, 5e5); + // Deposit and transition + await fixture.position.deposit(users[0].address, 1e6); + const beforeBalanceToken = await fixture.usdc.balanceOf(users[0].address); + const beforeBalanceShares = await fixture.position.balanceOf( + users[0].address ); - // TODO - Update so that the ethers factories play nice with types - vaultV4 = (await mockYearnV4Factory.deploy( - fixture.usdc.address, - await fixture.usdc.decimals() - )) as TestYVaultV4; - // Deploy a wrapped position - const wpFactory = await ethers.getContractFactory("YVaultV4AssetProxy"); - wpV4 = (await wpFactory.deploy( - vaultV4.address, - fixture.usdc.address, - "mock yearn v4", - "test" - )) as YVaultV4AssetProxy; - // set approvals for accounts - await fixture.usdc.approve(wpV4.address, ethers.constants.MaxUint256); - await fixture.usdc - .connect(users[1].user) - .approve(wpV4.address, ethers.constants.MaxUint256); - // deposit into the yearn vault so there's a supply - await fixture.usdc.approve(vaultV4.address, ethers.constants.MaxUint256); - await vaultV4.deposit(1e6, users[0].address); + await fixture.position.transition(newVault.address, 0); + // Withdraw and check balances + await fixture.position.withdraw(users[0].address, beforeBalanceShares, 0); + const afterBalanceToken = await fixture.usdc.balanceOf(users[0].address); + // Minus one to allow for rounding error + expect(afterBalanceToken.sub(beforeBalanceToken)).to.be.eq(1e6 - 1); }); - beforeEach(async () => { - await createSnapshot(provider); + it("has consistent price per share over upgrades", async () => { + const priceBefore = await fixture.position.getSharesToUnderlying(1e6); + await fixture.position.transition(newVault.address, 0); + const priceAfter = await fixture.position.getSharesToUnderlying(1e6); + // Allow some rounding + expect(priceAfter.sub(priceBefore).lt(10)).to.be.true; }); - afterEach(async () => { - // revert back to initial state after all tests pass - await restoreSnapshot(provider); + }); + + describe("Pause tests", async () => { + before(async () => { + // Pause the contract + await fixture.position.pause(true); }); - it("Won't deploy on a v3 wrapped position", async () => { - const wpFactory = await ethers.getContractFactory("YVaultAssetProxy"); - const tx = wpFactory.deploy( - vaultV4.address, - fixture.usdc.address, - "mock yearn v4", - "test" - ); - await expect(tx).to.be.revertedWith("Unsupported Version"); + it("Can't deposit", async () => { + const tx = fixture.position.deposit(users[0].address, 100); + await expect(tx).to.be.revertedWith("Paused"); }); - it("Reserve deposits", async () => { - // First deposit inits - await wpV4.reserveDeposit(1e6); - // Check that we got shares - let balance = await wpV4.reserveBalances(users[0].address); - expect(balance).to.be.eq(BigNumber.from(1e6)); - // Do a deposit to convert to shares - await wpV4.connect(users[1].user).deposit(users[1].address, 1e6); - // We want to change deposit rate - await fixture.usdc.transfer(vaultV4.address, 5e5); - // Now we deposit and check the price of deposit - await wpV4.reserveDeposit(1e6); - // Check the new balance - balance = await wpV4.reserveBalances(users[0].address); - // Magic number from rounding error on deposits - expect(balance).to.be.eq(BigNumber.from(1857144)); + it("Can't withdraw", async () => { + const tx = fixture.position.withdraw(users[0].address, 0, 0); + await expect(tx).to.be.revertedWith("Paused"); }); - it("withdraws with the correct ratio", async () => { - await wpV4.deposit(users[0].address, 1e6); - // Check that we got shares - const balance = await wpV4.balanceOf(users[0].address); - expect(balance).to.be.eq(BigNumber.from(1e6)); - // We want to change withdraw rate - await fixture.usdc.transfer(vaultV4.address, 5e5); - // withdraw - const balanceBefore = await fixture.usdc.balanceOf(users[0].address); - await wpV4.withdraw(users[0].address, balance, 0); - const balanceAfter = await fixture.usdc.balanceOf(users[0].address); - // check the withdraw amount - expect(balanceAfter.sub(balanceBefore)).to.be.eq(125e4); + it("Only useable by authorized", async () => { + const tx = fixture.position.connect(users[1].user).pause(false); + await expect(tx).to.be.revertedWith("Sender not Authorized"); }); }); }); From 74293f41c41793d9213f902bd35ae0d5b9867450 Mon Sep 17 00:00:00 2001 From: Paul Vienhage Date: Thu, 11 Nov 2021 20:46:15 +0100 Subject: [PATCH 2/5] Changed CC Pool LP withdraw to use an input of Shares (#216) * changed the withdraw function to use LP token inputs instead * comment updates * Changes the governance fee collection (#217) * fee collection update * explictly ban the governance address * test merge fixes --- contracts/ConvergentCurvePool.sol | 259 ++++++++------------- contracts/YVaultAssetProxy.sol | 11 +- contracts/test/TestConvergentCurvePool.sol | 15 +- test/convergentCurvePoolTests.ts | 130 ++++++----- test/wrappedPositionTest.ts | 26 ++- 5 files changed, 188 insertions(+), 253 deletions(-) diff --git a/contracts/ConvergentCurvePool.sol b/contracts/ConvergentCurvePool.sol index a554e555..82ea9727 100644 --- a/contracts/ConvergentCurvePool.sol +++ b/contracts/ConvergentCurvePool.sol @@ -31,10 +31,14 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { IVault private immutable _vault; bytes32 private immutable _poolId; - // The fees recorded during swaps. These will be 18 point not token decimal encoded + // The fees recorded during swaps, this is the total fees collected by LPs on all trades. + // These will be 18 point not token decimal encoded uint128 public feesUnderlying; - // This typing is a weird mismatch but we want to be able to hold a bool as well - uint120 public feesBond; + uint128 public feesBond; + // The fees which have been allocated to pay governance, a percent of LP fees on trades + // Since we don't have access to transfer they must be stored so governance can collect them later + uint128 public governanceFeesUnderlying; + uint128 public governanceFeesBond; // A bool to indicate if the contract is paused, stored with 'fees bond' bool public paused; // A mapping of who can pause @@ -166,7 +170,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { SwapRequest memory swapRequest, uint256 currentBalanceTokenIn, uint256 currentBalanceTokenOut - ) public override notPaused() returns (uint256) { + ) public override notPaused returns (uint256) { // Check that the sender is pool, we change state so must make // this check. require(msg.sender == address(_vault), "Non Vault caller"); @@ -234,7 +238,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { /// @param recipient The address which will receive lp tokens. /// @param currentBalances The current pool balances, sorted by address low to high. length 2 // @param latestBlockNumberUsed last block number unused in this pool - /// @param protocolSwapFee The percent of pool fees to be paid to the Balancer Protocol + /// @param protocolSwapFee no fee is collected on join only when they are paid to governance /// @param userData Abi encoded fixed length 2 array containing max inputs also sorted by /// address low to high /// @return amountsIn The actual amounts of token the vault should move to this pool @@ -250,7 +254,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { ) external override - notPaused() + notPaused returns ( uint256[] memory amountsIn, uint256[] memory dueProtocolFeeAmounts @@ -264,26 +268,22 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { currentBalances.length == 2 && maxAmountsIn.length == 2, "Invalid format" ); + require( + recipient != governance, + "Governance address LP would be locked" + ); // We must normalize the inputs to 18 point _normalizeSortedArray(currentBalances); _normalizeSortedArray(maxAmountsIn); - // Mint LP to the governance address. - // The {} zoning here helps solidity figure out the stack - { - ( - uint256 localFeeUnderlying, - uint256 localFeeBond - ) = _mintGovernanceLP(currentBalances); - dueProtocolFeeAmounts = new uint256[](2); + // This (1) removes governance fees and balancer fees which are collected but not paid + // from the current balances (2) adds any to be collected gov fees to state (3) calculates + // the fees to be paid to balancer on this call. + dueProtocolFeeAmounts = _feeAccounting( + currentBalances, + protocolSwapFee + ); - dueProtocolFeeAmounts[baseIndex] = localFeeUnderlying.mulDown( - protocolSwapFee - ); - dueProtocolFeeAmounts[bondIndex] = localFeeBond.mulDown( - protocolSwapFee - ); - } // Mint for the user amountsIn = _mintLP( maxAmountsIn[baseIndex], @@ -301,19 +301,19 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { /// It burns a proportional number of tokens compared to current LP pool, /// based on the minium output the user wants. /// @param poolId The balancer pool id, checked to ensure non erroneous vault call - // @param sender Unused by this pool but in interface - /// @param recipient The address which will receive the withdraw tokens. + /// @param sender The address which is the source of the LP token + // @param recipient Unused by this pool but in interface /// @param currentBalances The current pool balances, sorted by address low to high. length 2 // @param latestBlockNumberUsed last block number unused in this pool /// @param protocolSwapFee The percent of pool fees to be paid to the Balancer Protocol - /// @param userData Abi encoded fixed length 2 array containing min outputs also sorted by - /// address low to high + /// @param userData Abi encoded uint256 which is the number of LP tokens the user wants to + /// withdraw /// @return amountsOut The number of each token to send to the caller /// @return dueProtocolFeeAmounts The amounts of each token to pay as protocol fees function onExitPool( bytes32 poolId, + address sender, address, - address recipient, uint256[] memory currentBalances, uint256, uint256 protocolSwapFee, @@ -329,40 +329,33 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { // Default checks require(msg.sender == address(_vault), "Non Vault caller"); require(poolId == _poolId, "Wrong pool id"); - uint256[] memory minAmountsOut = abi.decode(userData, (uint256[])); - require( - currentBalances.length == 2 && minAmountsOut.length == 2, - "Invalid format" - ); + uint256 lpOut = abi.decode(userData, (uint256)); + // We have to convert to 18 decimals _normalizeSortedArray(currentBalances); - _normalizeSortedArray(minAmountsOut); - // Mint LP for the governance address. - // {} zones to help solidity figure out the stack - { - ( - uint256 localFeeUnderlying, - uint256 localFeeBond - ) = _mintGovernanceLP(currentBalances); + // This (1) removes governance fees and balancer fees which are collected but not paid + // from the current balances (2) adds any to be collected gov fees to state (3) calculates + // the fees to be paid to balancer on this call. + dueProtocolFeeAmounts = _feeAccounting( + currentBalances, + protocolSwapFee + ); - // Calculate the amount of fees for balancer to collect + // If governance is withdrawing they can get all of the fees + if (sender == governance) { + // Init the array + amountsOut = new uint256[](2); + // Governance withdraws the fees which have been paid to them + amountsOut[baseIndex] = uint256(governanceFeesUnderlying); + amountsOut[bondIndex] = uint256(governanceFeesBond); + } else { + // Calculate the user's proportion of the reserves + amountsOut = _burnLP(lpOut, currentBalances, sender); + // Balancer fees collected are zero dueProtocolFeeAmounts = new uint256[](2); - dueProtocolFeeAmounts[baseIndex] = localFeeUnderlying.mulDown( - protocolSwapFee - ); - dueProtocolFeeAmounts[bondIndex] = localFeeBond.mulDown( - protocolSwapFee - ); } - amountsOut = _burnLP( - minAmountsOut[baseIndex], - minAmountsOut[bondIndex], - currentBalances, - recipient - ); - // We need to convert the balancer outputs to token decimals instead of 18 _denormalizeSortedArray(amountsOut); _denormalizeSortedArray(dueProtocolFeeAmounts); @@ -578,120 +571,28 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { } } - /// @dev Burns at least enough LP tokens from the sender to produce - /// as set of minium outputs. - /// @param minOutputUnderlying The minimum output in underlying - /// @param minOutputBond The minimum output in the bond + /// @dev Burns a number of LP tokens and returns the amount of the pool which they own. + /// @param lpOut The minimum output in underlying /// @param currentBalances The current pool balances, sorted by address low to high. length 2 /// @param source The address to burn from. /// @return amountsReleased in address sorted order function _burnLP( - uint256 minOutputUnderlying, - uint256 minOutputBond, + uint256 lpOut, uint256[] memory currentBalances, address source ) internal returns (uint256[] memory amountsReleased) { - // Initialize the memory array with length - amountsReleased = new uint256[](2); - // We take in sorted token arrays to help the stack but - // use local names to improve readability - (uint256 reserveUnderlying, uint256 reserveBond) = _getSortedBalances( - currentBalances - ); - + // Load the number of LP token uint256 localTotalSupply = totalSupply(); - // Calculate the ratio of the minOutputUnderlying to reserve - uint256 underlyingPerBond = reserveUnderlying.divDown(reserveBond); - // If the ratio won't produce enough bond - if (minOutputUnderlying > minOutputBond.mulDown(underlyingPerBond)) { - // In this case we burn enough tokens to output 'minOutputUnderlying' - // which will be the total supply times the percent of the underlying - // reserve which this amount of underlying is. - uint256 burned = (minOutputUnderlying.mulDown(localTotalSupply)) - .divDown(reserveUnderlying); - _burnPoolTokens(source, burned); - // We return that we released 'minOutputUnderlying' and the number of bonds that - // preserves the reserve ratio - amountsReleased[baseIndex] = minOutputUnderlying; - amountsReleased[bondIndex] = minOutputUnderlying.divDown( - underlyingPerBond - ); - } else { - // Then the amount burned is the ratio of the minOutputBond - // to the reserve of bond times the total supply - uint256 burned = (minOutputBond.mulDown(localTotalSupply)).divDown( - reserveBond - ); - _burnPoolTokens(source, burned); - // We return that we released all of the minOutputBond - // and the number of underlying which preserves the reserve ratio - amountsReleased[baseIndex] = minOutputBond.mulDown( - underlyingPerBond - ); - amountsReleased[bondIndex] = minOutputBond; - } - } - - /// @dev Mints LP tokens from a percentage of the stored fees and then updates them - /// @param currentBalances The current pool balances, sorted by address low to high. length 2 - /// expects the inputs to be 18 point fixed - /// @return Returns the fee amounts as (feeUnderlying, feeBond) to avoid other sloads - function _mintGovernanceLP(uint256[] memory currentBalances) - internal - returns (uint256, uint256) - { - // Load and cast the stored fees - // Note - Because of sizes should only be one sload - uint256 localFeeUnderlying = uint256(feesUnderlying); - uint256 localFeeBond = uint256(feesBond); - if (percentFeeGov == 0) { - // We reset this state because it is expected that this function - // resets the amount to match what's consumed and in the zero fee case - // that's everything. - (feesUnderlying, feesBond) = (0, 0); - // Emit a fee tracking event - emit FeeCollection(localFeeUnderlying, localFeeBond, 0, 0); - // Return the used fees - return (localFeeUnderlying, localFeeBond); - } - - // Calculate the gov fee which is the assigned fees times the - // percent - uint256 govFeeUnderlying = localFeeUnderlying.mulDown(percentFeeGov); - uint256 govFeeBond = localFeeBond.mulDown(percentFeeGov); - // Mint the actual LP for gov address - uint256[] memory consumed = _mintLP( - govFeeUnderlying, - govFeeBond, - currentBalances, - governance - ); - // We calculate the actual fees used - uint256 usedFeeUnderlying = (consumed[baseIndex]).divDown( - percentFeeGov - ); - uint256 usedFeeBond = (consumed[bondIndex]).divDown(percentFeeGov); - // Calculate the remaining fees, note due to rounding errors they are likely to - // be true that usedFees + remainingFees > originalFees by a very small rounding error - // this is safe as with a bounded gov fee it never consumes LP funds. - uint256 remainingUnderlying = govFeeUnderlying - .sub(consumed[baseIndex]) - .divDown(percentFeeGov); - uint256 remainingBond = govFeeBond.sub(consumed[bondIndex]).divDown( - percentFeeGov - ); - // Emit fee tracking event - emit FeeCollection( - usedFeeUnderlying, - usedFeeBond, - remainingUnderlying, - remainingBond - ); - // Store the remaining fees - feesUnderlying = uint128(remainingUnderlying); - feesBond = uint120(remainingBond); - // We return the fees which were removed from storage - return (usedFeeUnderlying, usedFeeBond); + // Burn the LP tokens from the user + _burnPoolTokens(source, lpOut); + // Initialize the memory array with length 2 + amountsReleased = new uint256[](2); + // They get a percent ratio of the pool, div down will cause a very small + // rounding error loss. + amountsReleased[baseIndex] = (currentBalances[baseIndex].mulDown(lpOut)) + .divDown(localTotalSupply); + amountsReleased[bondIndex] = (currentBalances[bondIndex].mulDown(lpOut)) + .divDown(localTotalSupply); } /// @dev Calculates 1 - t @@ -736,6 +637,46 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { revert("Token request doesn't match stored"); } + /// @notice Mutates the input current balances array to remove fees paid to governance and balancer + /// Also resets the LP storage and realizes governance fees, returns the fees which are paid + /// to balancer + /// @param currentBalances The overall balances + /// @param balancerFee The percent of LP fees that balancer takes as a fee + /// @return dueProtocolFees The fees which will be paid to balancer on this join or exit + /// @dev WARNING - Solidity will implicitly cast a 'uint256[] calldata' to 'uint256[] memory' on function calls, but + /// since calldata is immutable mutations made in this call are discarded in future references to the + /// variable in other functions. 'currentBalances' must STRICTLY be of type uint256[] memory for this + /// function to work. + function _feeAccounting( + uint256[] memory currentBalances, + uint256 balancerFee + ) internal returns (uint256[] memory dueProtocolFees) { + // Load the total fees + uint256 localFeeUnderlying = uint256(feesUnderlying); + uint256 localFeeBond = uint256(feesBond); + dueProtocolFees = new uint256[](2); + // Calculate the balancer fee [this also implicitly returns this data] + dueProtocolFees[bondIndex] = localFeeUnderlying.mulDown(balancerFee); + dueProtocolFees[baseIndex] = localFeeBond.mulDown(balancerFee); + // Calculate the governance fee from total LP + uint256 govFeeBase = localFeeUnderlying.mulDown(percentFeeGov); + uint256 govFeeBond = localFeeBond.mulDown(percentFeeGov); + // Add the fees collected by gov to the stored ones + governanceFeesUnderlying += uint128(govFeeBase); + governanceFeesBond += uint128(govFeeBond); + // We subtract the amounts which are paid as fee but have not been collected. + // This leaves LPs with the deposits plus their amount of fees + currentBalances[baseIndex] = currentBalances[baseIndex] + .sub(dueProtocolFees[baseIndex]) + .sub(governanceFeesUnderlying); + currentBalances[bondIndex] = currentBalances[bondIndex] + .sub(dueProtocolFees[bondIndex]) + .sub(governanceFeesBond); + // Since all fees have been accounted for we reset the LP fees collected to zero + feesUnderlying = uint128(0); + feesBond = uint128(0); + } + /// @dev Turns a token which is either 'bond' or 'underlying' into 18 point decimal /// @param amount The amount of the token in native decimal encoding /// @param token The address of the token diff --git a/contracts/YVaultAssetProxy.sol b/contracts/YVaultAssetProxy.sol index a564da84..57c0a1c6 100644 --- a/contracts/YVaultAssetProxy.sol +++ b/contracts/YVaultAssetProxy.sol @@ -63,12 +63,7 @@ contract YVaultAssetProxy is WrappedPosition, Authorizable { /// @notice Makes the actual deposit into the yearn vault /// @return Tuple (the shares minted, amount underlying used) - function _deposit() - internal - override - notPaused() - returns (uint256, uint256) - { + function _deposit() internal override notPaused returns (uint256, uint256) { // Get the amount deposited uint256 amount = token.balanceOf(address(this)); @@ -96,7 +91,7 @@ contract YVaultAssetProxy is WrappedPosition, Authorizable { uint256 _shares, address _destination, uint256 - ) internal override notPaused() returns (uint256) { + ) internal override notPaused returns (uint256) { // If the conversion rate is non-zero we have upgraded and so our wrapped shares are // not one to one with the original shares. if (conversionRate != 0) { @@ -145,7 +140,7 @@ contract YVaultAssetProxy is WrappedPosition, Authorizable { /// @notice Allows an authorized address or the owner to pause this contract /// @param pauseStatus true for paused, false for not paused /// @dev the caller must be authorized - function pause(bool pauseStatus) external onlyAuthorized() { + function pause(bool pauseStatus) external onlyAuthorized { paused = pauseStatus; } diff --git a/contracts/test/TestConvergentCurvePool.sol b/contracts/test/TestConvergentCurvePool.sol index 7eb42bf4..e5d28646 100644 --- a/contracts/test/TestConvergentCurvePool.sol +++ b/contracts/test/TestConvergentCurvePool.sol @@ -38,17 +38,11 @@ contract TestConvergentCurvePool is ConvergentCurvePool { // Allows tests to burn LP tokens directly function burnLP( - uint256 outputUnderlying, - uint256 outputBond, + uint256 lpBurn, uint256[] memory currentBalances, address source ) public { - uint256[] memory outputs = _burnLP( - outputUnderlying, - outputBond, - currentBalances, - source - ); + uint256[] memory outputs = _burnLP(lpBurn, currentBalances, source); // We use this to return because returndata from state changing tx isn't easily accessible. emit UIntReturn(outputs[baseIndex]); emit UIntReturn(outputs[bondIndex]); @@ -72,11 +66,6 @@ contract TestConvergentCurvePool is ConvergentCurvePool { emit UIntReturn(amountsIn[bondIndex]); } - // Allows tests to access mint gov LP - function mintGovLP(uint256[] memory currentReserves) public { - _mintGovernanceLP(currentReserves); - } - // Allows tests to access the trade fee calculator function assignTradeFee( uint256 amountIn, diff --git a/test/convergentCurvePoolTests.ts b/test/convergentCurvePoolTests.ts index debe7c80..6d6bb496 100644 --- a/test/convergentCurvePoolTests.ts +++ b/test/convergentCurvePoolTests.ts @@ -299,72 +299,17 @@ describe("ConvergentCurvePool", function () { expect(totalSupply).to.be.eq(oneThousand.add(sixteenHundred)); }); - it("Internally Mints LP correctly for Governance", async function () { - await resetPool(); - let govBalanceStart = await poolContract.balanceOf(elementAddress); - const ten = ethers.utils.parseUnits("10", 18); - const five = ethers.utils.parseUnits("5", 18); - // We set the accumulated fees - await mineTx(poolContract.setFees(ten, five)); - // Set the current total supply to 100 lp tokens - await mineTx(poolContract.setLPBalance(elementAddress, ten.mul(ten))); - govBalanceStart = await poolContract.balanceOf(elementAddress); - // Mint governance lp - await mineTx(poolContract.mintGovLP([ten.mul(ten), five.mul(ten)])); - // We now check that all of the fees were consume - const feesUnderlying = await poolContract.feesUnderlying(); - const feesBond = await poolContract.feesBond(); - expect(newBigNumber(0)).to.be.eq(feesUnderlying); - expect(newBigNumber(0)).to.be.eq(feesBond); - // We check that the governance address got ten lp tokens - const govBalanceNew = await poolContract.balanceOf(elementAddress); - expect(ethers.utils.parseUnits("0.5", 18).add(govBalanceStart)).to.be.eq( - govBalanceNew - ); - }); - - // We test the mint functionality where the bond should be fully consumed - it("Internally Mints LP correctly for the bond max", async function () { - await resetPool(); - const oneThousand = ethers.utils.parseUnits("1000", 18); - // Set the current total supply to 1000 lp tokens - await mineTx(poolContract.setLPBalance(accounts[0].address, oneThousand)); - // We want a min of 500 underlying and 100 bond - const fiveHundred = ethers.utils.parseUnits("500", 18); - const result = await mineTx( - poolContract.burnLP( - fiveHundred, - fiveHundred.div(5), - [oneThousand, fiveHundred], - accounts[0].address - ) - ); - // The call should have released 500 underlying and 250 bond - const returned = result.events.filter( - (event) => event.event == "UIntReturn" - ); - expect(returned[0].data).to.be.eq(fiveHundred); - expect(returned[1].data).to.be.eq(fiveHundred.div(2)); - // The call should have burned 50% of the LP tokens to produce this - const balance = await poolContract.balanceOf(accounts[0].address); - expect(balance).to.be.eq(fiveHundred); - const totalSupply = await poolContract.totalSupply(); - expect(totalSupply).to.be.eq(fiveHundred); - }); - - // We test the mint functionality where the bond should be fully consumed - it("Internally Mints LP correctly for the underlying max", async function () { + // We test the burn functionality where the bond should be fully consumed + it("Internally Burns LP correctly for the underlying max", async function () { await resetPool(); const oneThousand = ethers.utils.parseUnits("1000", 18); // Set the current total supply to 1000 lp tokens await mineTx(poolContract.setLPBalance(accounts[0].address, oneThousand)); - // We want a min of 250 underlying and 250 bond const fiveHundred = ethers.utils.parseUnits("500", 18); const twoFifty = fiveHundred.div(2); const result = await mineTx( poolContract.burnLP( - twoFifty, - twoFifty, + fiveHundred, [oneThousand, fiveHundred], accounts[0].address ) @@ -603,11 +548,18 @@ describe("ConvergentCurvePool", function () { SECONDS_IN_YEAR, testVault.address, ethers.utils.parseEther("0.05"), - elementAddress, + balancerSigner.address, `Element ${baseAssetSymbol} - fy${baseAssetSymbol}`, `${baseAssetSymbol}-fy${baseAssetSymbol}` ); + beforeEach(async () => { + await createSnapshot(provider); + }); + afterEach(async () => { + await restoreSnapshot(provider); + }); + aliasedVault = TestConvergentCurvePool__factory.connect( testVault.address, tokenSigner @@ -650,9 +602,7 @@ describe("ConvergentCurvePool", function () { ); // Check the returned fees expect(data[1][0]).to.be.eq(ethers.utils.parseUnits("1", BASE_DECIMALS)); - expect(data[1][1]).to.be.eq( - ethers.utils.parseUnits("0.5", BOND_DECIMALS) - ); + expect(data[1][1]).to.be.eq(ethers.utils.parseUnits("1", BOND_DECIMALS)); // We run the call but state changing await aliasedVault.onJoinPool( poolId, @@ -666,9 +616,12 @@ describe("ConvergentCurvePool", function () { ); // We check the state expect(await poolContract.feesUnderlying()).to.be.eq(0); - expect(await poolContract.feesBond()).to.be.eq( - ethers.utils.parseEther("5") + expect(await poolContract.feesBond()).to.be.eq(0); + // Note swap fee = 0.05 implies 1/20 as ratio + expect(await poolContract.governanceFeesUnderlying()).to.be.eq( + ten.div(20) ); + expect(await poolContract.governanceFeesBond()).to.be.eq(ten.div(20)); // We run another trade to ensure fees are not charged when no lp // is minted data = await aliasedVault.callStatic.onJoinPool( @@ -685,6 +638,55 @@ describe("ConvergentCurvePool", function () { expect(data[1][0]).to.be.eq(0); expect(data[1][1]).to.be.eq(0); }); + it("Allows the governance to collect realized fees", async () => { + const poolId = await poolContract.getPoolId(); + // First create some pretend fees + const ten = ethers.utils.parseUnits("10", 18); + // Mint some lp to avoid init case + await poolContract.setLPBalance(tokenSigner.address, 1); + // We set the accumulated fees + await poolContract.setFees(ten, ten); + + const bondFirst = BigNumber.from(bondAssetContract.address).lt( + BigNumber.from(baseAssetContract.address) + ); + const bondIndex = bondFirst ? 0 : 1; + const baseIndex = bondFirst ? 1 : 0; + const reserves: BigNumberish[] = [0, 0]; + reserves[bondIndex] = ethers.utils.parseUnits("50", BOND_DECIMALS); + reserves[baseIndex] = ethers.utils.parseUnits("100", BASE_DECIMALS); + const lp_deposit: BigNumberish[] = [0, 0]; + lp_deposit[bondIndex] = ethers.utils.parseUnits("5", BOND_DECIMALS); + lp_deposit[baseIndex] = ethers.utils.parseUnits("10", BASE_DECIMALS); + // This call changes the state + await aliasedVault.onJoinPool( + poolId, + fakeAddress, + tokenSigner.address, + // Pool reserves are [100, 50] + reserves, + 0, + ethers.utils.parseEther("0.1"), + ethers.utils.defaultAbiCoder.encode(["uint256[]"], [lp_deposit]) + ); + // now we simulate a withdraw to see what the return values are + const data = await aliasedVault.callStatic.onExitPool( + poolId, + await poolContract.governance(), + fakeAddress, + reserves, + 0, + ethers.utils.parseEther("0.1"), + ethers.utils.defaultAbiCoder.encode(["uint256"], [0]) + ); + // we check that the amounts out are the whole fees + expect(data[0][bondIndex]).to.be.eq( + ethers.utils.parseUnits("0.5", BOND_DECIMALS) + ); + expect(data[0][baseIndex]).to.be.eq( + ethers.utils.parseUnits("0.5", BASE_DECIMALS) + ); + }); it("Blocks invalid vault calls", async () => { const poolId = await poolContract.getPoolId(); // First create some pretend fees diff --git a/test/wrappedPositionTest.ts b/test/wrappedPositionTest.ts index e677bf6f..e2ae80e0 100644 --- a/test/wrappedPositionTest.ts +++ b/test/wrappedPositionTest.ts @@ -31,7 +31,7 @@ describe("Wrapped Position", () => { users.map(async (userInfo) => { const { user } = userInfo; userInfo.address = await user.getAddress(); - await fixture.usdc.mint(userInfo.address, 6e6); + await fixture.usdc.mint(userInfo.address, 7e6); await fixture.usdc .connect(user) .approve(fixture.position.address, 12e6); @@ -91,22 +91,30 @@ describe("Wrapped Position", () => { }); describe("transfer", () => { it("should correctly transfer value", async () => { - /* At this point: - * User 1: 2 USDC deposited - * user 2: 2 USDC deposited - * User 3: 6 USDC deposited - */ + await fixture.position + .connect(users[3].user) + .deposit(users[3].address, 6e6); + // Update vault to 1 share = 1.1 USDC await fixture.yusdc.updateShares(); await fixture.position .connect(users[3].user) .transfer(users[1].address, 5e6); expect(await fixture.position.balanceOf(users[3].address)).to.equal(1e6); - expect(await fixture.position.balanceOf(users[1].address)).to.equal(7e6); + expect(await fixture.position.balanceOf(users[1].address)).to.equal(5e6); }); }); describe("withdraw", () => { it("should correctly withdraw value", async () => { + await fixture.position + .connect(users[1].user) + .deposit(users[1].address, 7e6); + await fixture.position + .connect(users[2].user) + .deposit(users[2].address, 2e6); + await fixture.position + .connect(users[3].user) + .deposit(users[3].address, 1e6); /* At this point: * User 1: 7 shares * user 2: 2 shares @@ -150,7 +158,7 @@ describe("Wrapped Position", () => { const totalUsdcBalance = usdcBalanceUser0 .add(usdcBalanceUser1) .add(usdcBalanceUser2); - expect(totalUsdcBalance).to.equal(ethers.BigNumber.from("19000000")); + expect(totalUsdcBalance).to.equal(ethers.BigNumber.from("21000000")); }); }); describe("Yearn migration", async () => { @@ -183,7 +191,7 @@ describe("Wrapped Position", () => { await fixture.position.transition(newVault.address, 0); const conversionRate = await fixture.position.conversionRate(); // Magic hex is 1.1 in 18 point fixed - expect(conversionRate).to.be.eq(BigNumber.from("0xF43FC2C04EE0000")); + expect(conversionRate).to.be.eq(BigNumber.from("0x10cac896d2390000")); }); it("Blocks non governance upgrades", async () => { From 1f6b4b90c7fc637a735b5e2683ed35ec9a0054c8 Mon Sep 17 00:00:00 2001 From: Paul Vienhage Date: Tue, 25 Jan 2022 18:59:07 +0100 Subject: [PATCH 3/5] Update contracts/ConvergentCurvePool.sol Co-authored-by: Daejun Park --- contracts/ConvergentCurvePool.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/ConvergentCurvePool.sol b/contracts/ConvergentCurvePool.sol index 82ea9727..4b95cee2 100644 --- a/contracts/ConvergentCurvePool.sol +++ b/contracts/ConvergentCurvePool.sol @@ -656,8 +656,8 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { uint256 localFeeBond = uint256(feesBond); dueProtocolFees = new uint256[](2); // Calculate the balancer fee [this also implicitly returns this data] - dueProtocolFees[bondIndex] = localFeeUnderlying.mulDown(balancerFee); - dueProtocolFees[baseIndex] = localFeeBond.mulDown(balancerFee); + dueProtocolFees[baseIndex] = localFeeUnderlying.mulDown(balancerFee); + dueProtocolFees[bondIndex] = localFeeBond.mulDown(balancerFee); // Calculate the governance fee from total LP uint256 govFeeBase = localFeeUnderlying.mulDown(percentFeeGov); uint256 govFeeBond = localFeeBond.mulDown(percentFeeGov); From 7bbacc8bb3d9ae0cb1c777c5e1b2c0062a3ecbd9 Mon Sep 17 00:00:00 2001 From: Paul Vienhage Date: Wed, 26 Jan 2022 14:18:13 +0000 Subject: [PATCH 4/5] audit fixes --- contracts/ConvergentCurvePool.sol | 21 +++++++++++---------- contracts/test/TestConvergentCurvePool.sol | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/contracts/ConvergentCurvePool.sol b/contracts/ConvergentCurvePool.sol index 4b95cee2..229c2d57 100644 --- a/contracts/ConvergentCurvePool.sol +++ b/contracts/ConvergentCurvePool.sol @@ -33,14 +33,14 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { // The fees recorded during swaps, this is the total fees collected by LPs on all trades. // These will be 18 point not token decimal encoded - uint128 public feesUnderlying; - uint128 public feesBond; + uint120 public feesUnderlying; + uint120 public feesBond; + // A bool to indicate if the contract is paused, stored with 'fees bond' + bool public paused; // The fees which have been allocated to pay governance, a percent of LP fees on trades // Since we don't have access to transfer they must be stored so governance can collect them later uint128 public governanceFeesUnderlying; uint128 public governanceFeesBond; - // A bool to indicate if the contract is paused, stored with 'fees bond' - bool public paused; // A mapping of who can pause mapping(address => bool) public pausers; // Stored records of governance tokens @@ -349,11 +349,12 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { // Governance withdraws the fees which have been paid to them amountsOut[baseIndex] = uint256(governanceFeesUnderlying); amountsOut[bondIndex] = uint256(governanceFeesBond); + // We now have to zero the governance fees + governanceFeesUnderlying = 0; + governanceFeesBond = 0; } else { // Calculate the user's proportion of the reserves amountsOut = _burnLP(lpOut, currentBalances, sender); - // Balancer fees collected are zero - dueProtocolFeeAmounts = new uint256[](2); } // We need to convert the balancer outputs to token decimals instead of 18 @@ -473,7 +474,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { amountOut.sub(amountIn) ); // we record that fee collected from the underlying - feesUnderlying += uint128(impliedYieldFee); + feesUnderlying += uint120(impliedYieldFee); // and return the adjusted input quote return amountIn.add(impliedYieldFee); } else { @@ -502,7 +503,7 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { amountIn.sub(amountOut) ); // we record the collected underlying fee - feesUnderlying += uint128(impliedYieldFee); + feesUnderlying += uint120(impliedYieldFee); // and then return the updated output quote return amountOut.sub(impliedYieldFee); } @@ -673,8 +674,8 @@ contract ConvergentCurvePool is IMinimalSwapInfoPool, BalancerPoolToken { .sub(dueProtocolFees[bondIndex]) .sub(governanceFeesBond); // Since all fees have been accounted for we reset the LP fees collected to zero - feesUnderlying = uint128(0); - feesBond = uint128(0); + feesUnderlying = uint120(0); + feesBond = uint120(0); } /// @dev Turns a token which is either 'bond' or 'underlying' into 18 point decimal diff --git a/contracts/test/TestConvergentCurvePool.sol b/contracts/test/TestConvergentCurvePool.sol index e5d28646..fdb5005c 100644 --- a/contracts/test/TestConvergentCurvePool.sol +++ b/contracts/test/TestConvergentCurvePool.sol @@ -93,7 +93,7 @@ contract TestConvergentCurvePool is ConvergentCurvePool { } // Allows tests to specify fees without making trades - function setFees(uint128 amountUnderlying, uint120 amountBond) public { + function setFees(uint120 amountUnderlying, uint120 amountBond) public { feesUnderlying = amountUnderlying; feesBond = amountBond; } From 8a08f2cc70ac40a86eed9fa3eca5debe12bad931 Mon Sep 17 00:00:00 2001 From: Paul Vienhage Date: Wed, 2 Mar 2022 16:38:22 -0500 Subject: [PATCH 5/5] order fixes to nexus tests --- test/wrappedCoveredPrincipalTokenTests.ts | 176 ++++++++++++---------- 1 file changed, 95 insertions(+), 81 deletions(-) diff --git a/test/wrappedCoveredPrincipalTokenTests.ts b/test/wrappedCoveredPrincipalTokenTests.ts index 2c44de03..49074d00 100644 --- a/test/wrappedCoveredPrincipalTokenTests.ts +++ b/test/wrappedCoveredPrincipalTokenTests.ts @@ -128,6 +128,10 @@ describe("WrappedCoveredPrincipalToken", function () { expect(await fixture.tranche.balanceOf(user2Address)).to.equal( initialBalance ); + + await coveredToken + .connect(signers[1]) + .addWrappedPosition(fixture.positionStub.address); }); after(async () => { @@ -137,7 +141,7 @@ describe("WrappedCoveredPrincipalToken", function () { it("should fail to add wrapped position because msg.sender is not the owner", async () => { const tx = coveredToken .connect(signers[2]) - .addWrappedPosition(fixture.positionStub.address); + .addWrappedPosition(signers[2].address); await expect(tx).to.be.revertedWith( "AccessControl: account 0x3c44cdddb6a900fa2b585dd299e03d12fa4293bc is missing role 0x41444d494e5f524f4c4500000000000000000000000000000000000000000000" ); @@ -155,12 +159,13 @@ describe("WrappedCoveredPrincipalToken", function () { await expect(tx).to.be.revertedWith("WFP:INVALID_WP"); }); - it("should successfully add the wrapped position", async () => { - await coveredToken - .connect(signers[1]) - .addWrappedPosition(fixture.positionStub.address); - expect((await coveredToken.allWrappedPositions()).length).to.equal(1); - }); + // it("should successfully add the wrapped position", async () => { + // await coveredToken + // .connect(signers[1]) + // .addWrappedPosition(fixture.positionStub.address); + // expect((await coveredToken.allWrappedPositions()).length).to.equal(1); + // expect(await coveredToken.isAllowedWp(fixture.positionStub.address)).to.equal(true); + // }); it("should fail to add wrapped position because it is already added", async () => { const tx = coveredToken @@ -203,83 +208,92 @@ describe("WrappedCoveredPrincipalToken", function () { await expect(tx).to.be.revertedWith("WFP:POSITION_NOT_EXPIRED"); }); - it("should failed to mint the wrapped covered token because allowance not provided", async () => { - const expirationTime = (await fixture.tranche.unlockTimestamp()).add(1); - advanceTime(provider, expirationTime.toNumber()); - const tx = coveredToken - .connect(user1) - .mint(tokenToMint, expiration, fixture.positionStub.address, { - spender: "0x0000000000000000000000000000000000000000", - value: 0, - deadline: 0, - v: 0, - r: ethers.utils.hexZeroPad("0x1f", 32), - s: ethers.utils.hexZeroPad("0x1f", 32), - }); - await expect(tx).to.be.revertedWith("ERC20: insufficient-allowance"); - }); + describe("Tests after time advance", async () => { + before(async () => { + const expirationTime = (await fixture.tranche.unlockTimestamp()).add(1); + advanceTime(provider, expirationTime.toNumber()); + }); - it("should successfully mint the wrapped covered token", async () => { - await fixture.tranche - .connect(user1) - .approve(coveredToken.address, initialBalance); - await coveredToken - .connect(user1) - .mint(tokenToMint, expiration, fixture.positionStub.address, { - spender: "0x0000000000000000000000000000000000000000", - value: 0, - deadline: 0, - v: 0, - r: ethers.utils.hexZeroPad("0x1f", 32), - s: ethers.utils.hexZeroPad("0x1f", 32), - }); - expect(await coveredToken.balanceOf(user1Address)).to.equal(tokenToMint); - expect(await fixture.tranche.balanceOf(user1Address)).to.equal(0); - }); + it("should failed to mint the wrapped covered token because allowance not provided", async () => { + const tx = coveredToken + .connect(user1) + .mint(tokenToMint, expiration, fixture.positionStub.address, { + spender: "0x0000000000000000000000000000000000000000", + value: 0, + deadline: 0, + v: 0, + r: ethers.utils.hexZeroPad("0x1f", 32), + s: ethers.utils.hexZeroPad("0x1f", 32), + }); + await expect(tx).to.be.revertedWith("ERC20: insufficient-allowance"); + }); - it("should failed to mint the wrapped covered token as allowance not provide because of invalid permit data", async () => { - const token = fixture.tranche as ERC20Permit; - const sig = await getPermitSignature( - token, - user1Address, - coveredToken.address, - initialBalance, - "1" - ); - const tx = coveredToken - .connect(user2) - .mint(tokenToMint, expiration, fixture.positionStub.address, { - spender: coveredToken.address, - value: initialBalance, - deadline: ethers.constants.MaxUint256, - v: sig.v, - r: sig.r, - s: sig.s, - }); - await expect(tx).to.be.revertedWith("ERC20: invalid-permit"); - }); + it("should successfully mint the wrapped covered token", async () => { + await fixture.tranche + .connect(user1) + .approve(coveredToken.address, initialBalance); + await coveredToken + .connect(user1) + .mint(tokenToMint, expiration, fixture.positionStub.address, { + spender: "0x0000000000000000000000000000000000000000", + value: 0, + deadline: 0, + v: 0, + r: ethers.utils.hexZeroPad("0x1f", 32), + s: ethers.utils.hexZeroPad("0x1f", 32), + }); + expect(await coveredToken.balanceOf(user1Address)).to.equal( + tokenToMint + ); + expect(await fixture.tranche.balanceOf(user1Address)).to.equal(0); + }); - it("should successfully mint the wrapped covered token using permit data", async () => { - const token = fixture.tranche as ERC20Permit; - const sig = await getPermitSignature( - token, - user2Address, - coveredToken.address, - initialBalance, - "1" - ); - await coveredToken - .connect(user2) - .mint(tokenToMint, expiration, fixture.positionStub.address, { - spender: coveredToken.address, - value: initialBalance, - deadline: ethers.constants.MaxUint256, - v: sig.v, - r: sig.r, - s: sig.s, - }); - expect(await coveredToken.balanceOf(user2Address)).to.equal(tokenToMint); - expect(await fixture.tranche.balanceOf(user2Address)).to.equal(0); + it("should failed to mint the wrapped covered token as allowance not provide because of invalid permit data", async () => { + const token = fixture.tranche as ERC20Permit; + const sig = await getPermitSignature( + token, + user1Address, + coveredToken.address, + initialBalance, + "1" + ); + const tx = coveredToken + .connect(user2) + .mint(tokenToMint, expiration, fixture.positionStub.address, { + spender: coveredToken.address, + value: initialBalance, + deadline: ethers.constants.MaxUint256, + v: sig.v, + r: sig.r, + s: sig.s, + }); + await expect(tx).to.be.revertedWith("ERC20: invalid-permit"); + }); + + it("should successfully mint the wrapped covered token using permit data", async () => { + const token = fixture.tranche as ERC20Permit; + const sig = await getPermitSignature( + token, + user2Address, + coveredToken.address, + initialBalance, + "1" + ); + await coveredToken + .connect(user2) + .mint(tokenToMint, expiration, fixture.positionStub.address, { + spender: coveredToken.address, + value: initialBalance, + deadline: ethers.constants.MaxUint256, + v: sig.v, + r: sig.r, + s: sig.s, + }); + expect(await coveredToken.balanceOf(user2Address)).to.equal( + tokenToMint + ); + expect(await fixture.tranche.balanceOf(user2Address)).to.equal(0); + }); }); it("should verify the getters output", async () => {