From dd99e06ce93114490f33d7fe62f10f2e8d7684e4 Mon Sep 17 00:00:00 2001 From: Paul Vienhage Date: Wed, 29 Sep 2021 22:04:06 +0100 Subject: [PATCH] pause and admin upgrade --- contracts/ConvergentCurvePool.sol | 52 +++++++- contracts/YVaultAssetProxy.sol | 113 ++++++++++++++++-- contracts/factories/ConvergentPoolFactory.sol | 7 +- contracts/test/TestConvergentCurvePool.sol | 5 +- contracts/test/TestYVault.sol | 4 + test/convergentCurvePoolTests.ts | 57 ++++++++- test/erc20.ts | 2 +- test/helpers/deployer.ts | 10 +- test/wrappedPositionTest.ts | 111 +++++++++++++++++ 9 files changed, 340 insertions(+), 21 deletions(-) diff --git a/contracts/ConvergentCurvePool.sol b/contracts/ConvergentCurvePool.sol index 8c94eaa0..bfb22ae7 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 a99e2d73..646aa46e 100644 --- a/contracts/YVaultAssetProxy.sol +++ b/contracts/YVaultAssetProxy.sol @@ -6,12 +6,22 @@ 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 { - // the yearn vault and how many decimals it's deposit receipt tokens are - 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; /// @notice Constructs this contract and stores needed data @@ -20,14 +30,25 @@ contract YVaultAssetProxy is WrappedPosition { /// 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( @@ -36,21 +57,40 @@ contract YVaultAssetProxy is WrappedPosition { ); } + /// @notice Checks that the contract has not been paused + modifier notPaused() { + require(!paused, "Paused"); + _; + } + /// @notice Makes the actual deposit into the yearn vault /// @return Tuple (the shares minted, amount underlying used) - function _deposit() internal override returns (uint256, uint256) { + function _deposit() + internal + override + notPaused() + returns (uint256, uint256) + { // Get the amount deposited uint256 amount = token.balanceOf(address(this)); // Deposit and get the shares that were minted to this 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; + } + // Return the amount of shares the user has produced, and the amount used for it. return (shares, amount); } /// @notice Withdraw the number of shares - /// @param _shares The number of shares to withdraw + /// @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 /// @return returns the amount of funds freed by doing a yearn withdraw @@ -58,7 +98,14 @@ contract YVaultAssetProxy is WrappedPosition { uint256 _shares, address _destination, uint256 - ) internal override 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) { + // 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; + } // 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. @@ -77,6 +124,11 @@ contract YVaultAssetProxy is WrappedPosition { view 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); } @@ -91,4 +143,49 @@ contract YVaultAssetProxy is WrappedPosition { token.approve(address(vault), 0); token.approve(address(vault), type(uint256).max); } + + /// @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 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 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/factories/ConvergentPoolFactory.sol b/contracts/factories/ConvergentPoolFactory.sol index 6225982a..75ea3ce2 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 a1599b95..c1d78231 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 6b0018fc..88f7394c 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 1c5d09e6..5404b672 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/erc20.ts b/test/erc20.ts index 6aa02d77..c450e7bf 100644 --- a/test/erc20.ts +++ b/test/erc20.ts @@ -105,7 +105,7 @@ describe("erc20", function () { r, s ); - await expect(tx).to.be.revertedWith("revert ERC20: invalid-permit"); + await expect(tx).to.be.revertedWith("ERC20: invalid-permit"); }); }); 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/wrappedPositionTest.ts b/test/wrappedPositionTest.ts index 3092fbfa..e677bf6f 100644 --- a/test/wrappedPositionTest.ts +++ b/test/wrappedPositionTest.ts @@ -5,6 +5,9 @@ import { ethers, waffle } from "hardhat"; import { FixtureInterface, loadFixture } from "./helpers/deployer"; import { createSnapshot, restoreSnapshot } from "./helpers/snapshots"; +import { TestYVault } from "../typechain/TestYVault"; +import { TestYVault__factory } from "../typechain/factories/TestYVault__factory"; + const { provider } = waffle; describe("Wrapped Position", () => { @@ -150,4 +153,112 @@ describe("Wrapped Position", () => { expect(totalUsdcBalance).to.equal(ethers.BigNumber.from("19000000")); }); }); + 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("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 + ); + await expect(tx).to.be.revertedWith("Not enough output"); + }); + + 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 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 + ); + 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); + }); + + 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; + }); + }); + + describe("Pause tests", async () => { + before(async () => { + // Pause the contract + await fixture.position.pause(true); + }); + + it("Can't deposit", async () => { + const tx = fixture.position.deposit(users[0].address, 100); + await expect(tx).to.be.revertedWith("Paused"); + }); + + it("Can't withdraw", async () => { + const tx = fixture.position.withdraw(users[0].address, 0, 0); + await expect(tx).to.be.revertedWith("Paused"); + }); + + 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"); + }); + }); });