From f532a2a38a043c93ed8278026b4149a1452a4efd Mon Sep 17 00:00:00 2001 From: Akshat Mittal Date: Tue, 10 Sep 2024 03:56:52 +0530 Subject: [PATCH 01/14] Test Fix --- test/plugins/individual-collateral/collateralTests.ts | 2 +- .../individual-collateral/ethena/USDeFiatCollateral.test.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/test/plugins/individual-collateral/collateralTests.ts b/test/plugins/individual-collateral/collateralTests.ts index 8dbce3ee93..8f62c5ffa9 100644 --- a/test/plugins/individual-collateral/collateralTests.ts +++ b/test/plugins/individual-collateral/collateralTests.ts @@ -150,7 +150,7 @@ export default function fn( let ctx: X let alice: SignerWithAddress - let chainId: number + let chainId: string let collateral: TestICollateral let chainlinkFeed: MockV3Aggregator diff --git a/test/plugins/individual-collateral/ethena/USDeFiatCollateral.test.ts b/test/plugins/individual-collateral/ethena/USDeFiatCollateral.test.ts index a1c1e10fc6..9624aaa70a 100644 --- a/test/plugins/individual-collateral/ethena/USDeFiatCollateral.test.ts +++ b/test/plugins/individual-collateral/ethena/USDeFiatCollateral.test.ts @@ -216,6 +216,7 @@ const opts = { chainlinkDefaultAnswer, itIsPricedByPeg: true, resetFork, + toleranceDivisor: bn('1e8'), } collateralTests(opts) From 72a9c2d854766ec8085ab03936fe4eaace5f1e75 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 09:42:32 -0400 Subject: [PATCH 02/14] prevent batch auction cancellations during final 10% of auction (#1203) --- contracts/plugins/trading/GnosisTrade.sol | 10 +++++- test/integration/EasyAuction.test.ts | 41 +++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/contracts/plugins/trading/GnosisTrade.sol b/contracts/plugins/trading/GnosisTrade.sol index 794f5ff2b6..082287d917 100644 --- a/contracts/plugins/trading/GnosisTrade.sol +++ b/contracts/plugins/trading/GnosisTrade.sol @@ -24,6 +24,9 @@ contract GnosisTrade is ITrade, Versioned { TradeKind public constant KIND = TradeKind.BATCH_AUCTION; uint256 public constant FEE_DENOMINATOR = 1000; + // Can only cancel order in first 90% of the auction + uint192 public constant CANCEL_WINDOW = 9e17; // {1} first 90% of auction + // Upper bound for the max number of orders we're happy to have the auction clear in; // When we have good price information, this determines the minimum buy amount per order. uint96 public constant MAX_ORDERS = 5000; // bounded to avoid going beyond block gas limit @@ -141,10 +144,15 @@ contract GnosisTrade is ITrade, Versioned { // amount is > 0 and < type(uint256).max. AllowanceLib.safeApproveFallbackToMax(address(sell), address(gnosis), req.sellAmount); + // Can only cancel within the CANCEL_WINDOW + uint48 cancellationEndTime = uint48( + block.timestamp + (batchAuctionLength * CANCEL_WINDOW) / FIX_ONE + ); + auctionId = gnosis.initiateAuction( sell, buy, - endTime, + cancellationEndTime, endTime, _sellAmount, minBuyAmount, diff --git a/test/integration/EasyAuction.test.ts b/test/integration/EasyAuction.test.ts index 0a352269bc..92520911b4 100644 --- a/test/integration/EasyAuction.test.ts +++ b/test/integration/EasyAuction.test.ts @@ -363,6 +363,47 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function expect(await rsr.balanceOf(backingManager.address)).to.equal(0) }) + it('cannot cancel in last 10% of auction', async () => { + // Place 2 orders + const bidAmt = buyAmt.add(1) + await token0.connect(addr1).approve(easyAuction.address, bidAmt.mul(3)) + await easyAuction + .connect(addr1) + .placeSellOrders(auctionId, [sellAmt], [bidAmt], [QUEUE_START], ethers.constants.HashZero) + await easyAuction + .connect(addr1) + .placeSellOrders( + auctionId, + [sellAmt], + [bidAmt.mul(2)], + [QUEUE_START], + ethers.constants.HashZero + ) + + // Advance halfway + await advanceTime(config.batchAuctionLength.div(2).toString()) + + // Cancel successfully + const OrderHelperFactory = await ethers.getContractFactory('IterableOrderedOrderSetWrapper') + const orderHelper = await OrderHelperFactory.deploy() + const userId = await easyAuction.callStatic.getUserId(addr1.address) + const order = await orderHelper.encodeOrder(userId, sellAmt, bidAmt) + await easyAuction.connect(addr1).cancelSellOrders(auctionId, [order]) + + // Advance near end + await advanceTime(config.batchAuctionLength.div(2).sub(10).toString()) + + // Cannot cancel + const order2 = await orderHelper.encodeOrder(userId, sellAmt, bidAmt.mul(2)) + await expect( + easyAuction.connect(addr1).cancelSellOrders(auctionId, [order2]) + ).to.be.revertedWith('no longer in order placement and cancelation phase') + + // End auction + await advanceTime(config.batchAuctionLength.div(2).toString()) + await easyAuction.settleAuction(auctionId) + }) + it('full volume -- bid at 2x price', async () => { const bidAmt = buyAmt.add(1) sellAmt = sellAmt.div(2) From 57456f9909fcba45b94f945654773fb189b8cee0 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 09:43:19 -0400 Subject: [PATCH 03/14] enable governance to call resetStakes() (#1193) --- contracts/plugins/governance/Governance.sol | 2 +- test/Governance.test.ts | 70 +++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/contracts/plugins/governance/Governance.sol b/contracts/plugins/governance/Governance.sol index 797d818fa7..d4a9665823 100644 --- a/contracts/plugins/governance/Governance.sol +++ b/contracts/plugins/governance/Governance.sol @@ -140,8 +140,8 @@ contract Governance is bytes[] memory calldatas, bytes32 descriptionHash ) internal override(Governor, GovernorTimelockControl) { - super._execute(proposalId, targets, values, calldatas, descriptionHash); require(startedInSameEra(proposalId), "new era"); + super._execute(proposalId, targets, values, calldatas, descriptionHash); } function _cancel( diff --git a/test/Governance.test.ts b/test/Governance.test.ts index 3b5e051fbc..29a7a87253 100644 --- a/test/Governance.test.ts +++ b/test/Governance.test.ts @@ -4,6 +4,7 @@ import { expect } from 'chai' import { BigNumber } from 'ethers' import { ethers } from 'hardhat' import { IConfig } from '../common/configuration' +import { setStorageAt } from '@nomicfoundation/hardhat-network-helpers' import { ProposalState, ZERO_ADDRESS, @@ -1185,5 +1186,74 @@ describeP1(`Governance - P${IMPLEMENTATION}`, () => { // Check value was not updated expect(await governor.votingDelay()).to.equal(VOTING_DELAY) }) + + it('Should be able to call resetStakes() -- regression test 09/04/2024', async () => { + // Context: https://github.com/code-423n4/2024-07-reserve-findings/issues/36 + + const era = await stRSRVotes.currentEra() + + await setStorageAt(stRSR.address, 263, fp('1e-6')) // MIN_SAFE_STAKE_RATE + + const encodedCall = stRSR.interface.encodeFunctionData('resetStakes') + + // Propose + const proposeTx = await governor + .connect(addr1) + .propose([stRSR.address], [0], [encodedCall], proposalDescription) + + const proposeReceipt = await proposeTx.wait(1) + const proposalId = proposeReceipt.events![0].args!.proposalId + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Pending) + + // Advance time to start voting + await advanceBlocks(VOTING_DELAY + 1) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Active) + + let voteWay = 1 // for + + // vote + await governor.connect(addr1).castVote(proposalId, voteWay) + await advanceBlocks(1) + + // Quorum should be equal to cast votes + const expectedQuorum = stkAmt1.mul(2).mul(QUORUM_PERCENTAGE).div(100) + expect(await governor.quorum((await getLatestBlockTimestamp()) - 1)).to.equal(expectedQuorum) + + voteWay = 2 // abstain + await governor.connect(addr2).castVoteWithReason(proposalId, voteWay, 'I abstain') + await advanceBlocks(1) + + // Quorum should be equal to sum of abstain + for votes + expect(await governor.quorum((await getLatestBlockTimestamp()) - 1)).to.equal(expectedQuorum) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Active) + + // Advance time till voting is complete + await advanceBlocks(VOTING_PERIOD + 1) + + // Finished voting - Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Succeeded) + + // Queue proposal + await governor.connect(addr1).queue([stRSR.address], [0], [encodedCall], proposalDescHash) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Queued) + + // Advance time required by timelock + await advanceTime(MIN_DELAY + 1) + await advanceBlocks(1) + + // Execute -- regression test, should not revert + await governor.connect(addr1).execute([stRSR.address], [0], [encodedCall], proposalDescHash) + + // Check era changed + expect(await stRSRVotes.currentEra()).to.equal(era.add(1)) + }) }) }) From 8697de85329be635903f49dbf7992ecfab9c8933 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 09:44:55 -0400 Subject: [PATCH 04/14] accmumulate throttles during dissolve()/mint() (#1197) --- contracts/p0/RToken.sol | 4 ++++ contracts/p1/RToken.sol | 13 ++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/contracts/p0/RToken.sol b/contracts/p0/RToken.sol index c30757f96b..ec3ba5ecd9 100644 --- a/contracts/p0/RToken.sol +++ b/contracts/p0/RToken.sol @@ -267,6 +267,8 @@ contract RTokenP0 is ComponentP0, ERC20PermitUpgradeable, IRToken { /// @custom:protected function mint(uint192 baskets) external exchangeRateIsValidAfter { require(_msgSender() == address(main.backingManager()), "not backing manager"); + issuanceThrottle.useAvailable(totalSupply(), 0); + redemptionThrottle.useAvailable(totalSupply(), 0); _scaleUp(address(main.backingManager()), baskets); } @@ -285,6 +287,8 @@ contract RTokenP0 is ComponentP0, ERC20PermitUpgradeable, IRToken { /// @custom:protected function dissolve(uint256 amount) external exchangeRateIsValidAfter { require(_msgSender() == address(main.backingManager()), "not backing manager"); + issuanceThrottle.useAvailable(totalSupply(), 0); + redemptionThrottle.useAvailable(totalSupply(), 0); _scaleDown(_msgSender(), amount); } diff --git a/contracts/p1/RToken.sol b/contracts/p1/RToken.sol index e6350c81ad..63308e58ea 100644 --- a/contracts/p1/RToken.sol +++ b/contracts/p1/RToken.sol @@ -355,7 +355,12 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { // BU exchange rate cannot decrease, and it can only increase when < FIX_ONE. function mint(uint192 baskets) external { require(_msgSender() == address(backingManager), "not backing manager"); - _scaleUp(address(backingManager), baskets, totalSupply()); + uint256 supply = totalSupply(); + + // Accumulate the throttle before the supply change + issuanceThrottle.useAvailable(supply, 0); + redemptionThrottle.useAvailable(supply, 0); + _scaleUp(address(backingManager), baskets, supply); } /// Melt a quantity of RToken from the caller's account, increasing the basket rate @@ -372,6 +377,7 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { require(caller == address(furnace), "furnace only"); _burn(caller, amtRToken); emit Melted(amtRToken); + // do not update throttles: melting is frequent and always small } /// Burn an amount of RToken from caller's account and scale basketsNeeded down @@ -387,6 +393,11 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { function dissolve(uint256 amount) external { address caller = _msgSender(); require(caller == address(backingManager), "not backing manager"); + uint256 supply = totalSupply(); + + // Accumulate the throttle before the supply change + issuanceThrottle.useAvailable(supply, 0); + redemptionThrottle.useAvailable(supply, 0); _scaleDown(caller, amount); } From f90e0e0731808e07293f601a222986b775f5de02 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 09:49:41 -0400 Subject: [PATCH 05/14] make future withdrawals unimpacted by previous cancellations (#1194) --- contracts/p0/StRSR.sol | 3 +-- contracts/p1/StRSR.sol | 4 +++- test/ZZStRSR.test.ts | 43 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/contracts/p0/StRSR.sol b/contracts/p0/StRSR.sol index 814a4a94e7..4a3c0a1da9 100644 --- a/contracts/p0/StRSR.sol +++ b/contracts/p0/StRSR.sol @@ -272,8 +272,7 @@ contract StRSRP0 is IStRSR, ComponentP0, EIP712Upgradeable { uint256 i = start; for (; i < endId; i++) { total += queue[i].rsrAmount; - queue[i].rsrAmount = 0; - queue[i].stakeAmount = 0; + delete queue[i]; } // Execute accumulated withdrawals diff --git a/contracts/p1/StRSR.sol b/contracts/p1/StRSR.sol index 5a6c001229..6e7b1e6f55 100644 --- a/contracts/p1/StRSR.sol +++ b/contracts/p1/StRSR.sol @@ -654,7 +654,9 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab index = queue.length; uint192 oldDrafts = index != 0 ? queue[index - 1].drafts : 0; - uint64 lastAvailableAt = index != 0 ? queue[index - 1].availableAt : 0; + uint64 lastAvailableAt = index != 0 && firstRemainingDraft[draftEra][account] < index + ? queue[index - 1].availableAt + : 0; availableAt = uint64(block.timestamp) + unstakingDelay; if (lastAvailableAt > availableAt) { availableAt = lastAvailableAt; diff --git a/test/ZZStRSR.test.ts b/test/ZZStRSR.test.ts index 6fbcd2b3d4..06fba12cc2 100644 --- a/test/ZZStRSR.test.ts +++ b/test/ZZStRSR.test.ts @@ -552,6 +552,49 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => { .withArgs(0, 2, addr1.address, amount.div(2), amount, anyValue) }) + it('Should not impact future withdrawals after cancellation -- regression test 9/04/2024', async () => { + // Context: https://github.com/code-423n4/2024-07-reserve-findings/issues/18 + + // old unstakingDelay is 1 day + const oldUnstakingDelay = 3600 * 24 + await stRSR.connect(owner).setUnstakingDelay(oldUnstakingDelay) + const amount: BigNumber = bn('100e18') + await rsr.connect(addr1).approve(stRSR.address, amount) + await stRSR.connect(addr1).stake(amount) + + const draftEra = 1 + const availableAtOfFirst = (await getLatestBlockTimestamp()) + oldUnstakingDelay + 1 + /** + * Unstaking request enter a queue, and withdrawal become available 1 day later + */ + await expect(stRSR.connect(addr1).unstake(amount)) + .emit(stRSR, 'UnstakingStarted') + .withArgs(0, draftEra, addr1.address, amount, amount, availableAtOfFirst) + + /** + * Cancel the unstaking to eliminate any pending withdrawals + */ + await stRSR.connect(addr1).cancelUnstake(1) + + // new unstakingDelay is 1 hour + const newUnstakingDelay = 3600 + await stRSR.connect(owner).setUnstakingDelay(newUnstakingDelay) + + await rsr.connect(addr2).approve(stRSR.address, amount) + await stRSR.connect(addr2).stake(amount) + + const availableAtOfFirstOfUser2 = (await getLatestBlockTimestamp()) + newUnstakingDelay + 1 + /** + * Unstaking request enter a queue. Withdrawal should become available 1 hour later for both users + */ + await expect(stRSR.connect(addr2).unstake(amount)) + .emit(stRSR, 'UnstakingStarted') + .withArgs(0, draftEra, addr2.address, amount, amount, availableAtOfFirstOfUser2) + await expect(stRSR.connect(addr1).unstake(amount)) + .emit(stRSR, 'UnstakingStarted') + .withArgs(1, draftEra, addr1.address, amount, amount, availableAtOfFirstOfUser2 + 1) + }) + it('Should create Pending withdrawal when unstaking', async () => { const amount: BigNumber = bn('1000e18') From f8e5f5c27c6be3ba311f3622c1608be87288d3f3 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 09:50:01 -0400 Subject: [PATCH 06/14] prevent distributor configuration to RToken/RSR contracts directly (#1191) --- contracts/p0/Distributor.sol | 6 +++++- contracts/p1/Distributor.sol | 6 +++++- test/Revenues.test.ts | 18 ++++++++++++++++-- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/contracts/p0/Distributor.sol b/contracts/p0/Distributor.sol index ade5d11df2..1f2a18700f 100644 --- a/contracts/p0/Distributor.sol +++ b/contracts/p0/Distributor.sol @@ -139,7 +139,11 @@ contract DistributorP0 is ComponentP0, IDistributor { require(dest != address(0), "dest cannot be zero"); require( dest != address(main.furnace()) && dest != address(main.stRSR()), - "destination can not be furnace or strsr directly" + "destination cannot be furnace or strsr directly" + ); + require( + dest != address(main.rsr()) && dest != address(main.rToken()), + "destination cannot be rsr or rToken" ); require(dest != address(main.daoFeeRegistry()), "destination cannot be daoFeeRegistry"); if (dest == FURNACE) require(share.rsrDist == 0, "Furnace must get 0% of RSR"); diff --git a/contracts/p1/Distributor.sol b/contracts/p1/Distributor.sol index 8040eeb03f..11b914adaa 100644 --- a/contracts/p1/Distributor.sol +++ b/contracts/p1/Distributor.sol @@ -241,7 +241,11 @@ contract DistributorP1 is ComponentP1, IDistributor { require(dest != address(0), "dest cannot be zero"); require( dest != address(furnace) && dest != address(stRSR), - "destination can not be furnace or strsr directly" + "destination cannot be furnace or strsr directly" + ); + require( + dest != address(rsr) && dest != address(rToken), + "destination cannot be rsr or rToken" ); require(dest != address(main.daoFeeRegistry()), "destination cannot be daoFeeRegistry"); if (dest == FURNACE) require(share.rsrDist == 0, "Furnace must get 0% of RSR"); diff --git a/test/Revenues.test.ts b/test/Revenues.test.ts index 0babf9da13..fb984ce952 100644 --- a/test/Revenues.test.ts +++ b/test/Revenues.test.ts @@ -361,14 +361,28 @@ describe(`Revenues - P${IMPLEMENTATION}`, () => { distributor .connect(owner) .setDistribution(furnace.address, { rTokenDist: bn(5), rsrDist: bn(5) }) - ).to.be.revertedWith('destination can not be furnace or strsr directly') + ).to.be.revertedWith('destination cannot be furnace or strsr directly') // Cannot set StRSR as beneficiary await expect( distributor .connect(owner) .setDistribution(stRSR.address, { rTokenDist: bn(5), rsrDist: bn(5) }) - ).to.be.revertedWith('destination can not be furnace or strsr directly') + ).to.be.revertedWith('destination cannot be furnace or strsr directly') + + // Cannot set RSR as beneficiary + await expect( + distributor + .connect(owner) + .setDistribution(rsr.address, { rTokenDist: bn(5), rsrDist: bn(5) }) + ).to.be.revertedWith('destination cannot be rsr or rToken') + + // Cannot set RToken as beneficiary + await expect( + distributor + .connect(owner) + .setDistribution(rToken.address, { rTokenDist: bn(5), rsrDist: bn(5) }) + ).to.be.revertedWith('destination cannot be rsr or rToken') }) itP1('Should not allow to set Dao fee explicitly', async () => { From 03c4a6cc69cc654d66a4830cea8a7540d3cfdb65 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 10:13:13 -0400 Subject: [PATCH 07/14] give governance a way to forcibly settle an auction, ie in case of censorship (#1201) --- contracts/interfaces/ITrading.sol | 6 ++++ contracts/p0/BackingManager.sol | 11 +++++- contracts/p0/mixins/Trading.sol | 15 +++++++- contracts/p1/BackingManager.sol | 11 +++++- contracts/p1/mixins/Trading.sol | 15 +++++++- test/scenario/BadERC20.test.ts | 57 +++++++++++++++++-------------- 6 files changed, 86 insertions(+), 29 deletions(-) diff --git a/contracts/interfaces/ITrading.sol b/contracts/interfaces/ITrading.sol index 6fc380b6b3..842a12487d 100644 --- a/contracts/interfaces/ITrading.sol +++ b/contracts/interfaces/ITrading.sol @@ -43,6 +43,12 @@ interface ITrading is IComponent, IRewardableComponent { uint256 buyAmount ); + /// Forcibly settle a trade, losing all value + /// Should only be called in case of censorship + /// @param trade The trade address itself + /// @custom:governance + function forceSettleTrade(ITrade trade) external; + /// Settle a single trade, expected to be used with multicall for efficient mass settlement /// @param sell The sell token in the trade /// @return The trade settled diff --git a/contracts/p0/BackingManager.sol b/contracts/p0/BackingManager.sol index 0bcb1b48c6..6bbd2c2a61 100644 --- a/contracts/p0/BackingManager.sol +++ b/contracts/p0/BackingManager.sol @@ -246,7 +246,16 @@ contract BackingManagerP0 is TradingP0, IBackingManager { assert(main.basketHandler().fullyCollateralized()); } - // === Setters === + // === Governance === + + /// Forcibly settle a trade, losing all value + /// Should only be called in case of censorship + /// @param trade The trade address itself + /// @custom:governance + function forceSettleTrade(ITrade trade) public override(TradingP0, ITrading) governance { + super.forceSettleTrade(trade); + delete tokensOut[trade.sell()]; + } /// @custom:governance function setTradingDelay(uint48 val) public governance { diff --git a/contracts/p0/mixins/Trading.sol b/contracts/p0/mixins/Trading.sol index 52fc993eea..a8e64d2a7f 100644 --- a/contracts/p0/mixins/Trading.sol +++ b/contracts/p0/mixins/Trading.sol @@ -94,7 +94,20 @@ abstract contract TradingP0 is RewardableP0, ITrading { ); } - // === Setters === + // === Governance === + + /// Forcibly settle a trade, losing all value + /// Should only be called in case of censorship + /// @param trade The trade address itself + /// @custom:governance + function forceSettleTrade(ITrade trade) public virtual governance { + // should not call any ERC20 functions, in case bricked + + IERC20Metadata sell = trade.sell(); + delete trades[sell]; + tradesOpen--; + emit TradeSettled(trade, sell, trade.buy(), 0, 0); + } /// @custom:governance function setMaxTradeSlippage(uint192 val) public governance { diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol index 6f8c8b2ff6..4724cb9c36 100644 --- a/contracts/p1/BackingManager.sol +++ b/contracts/p1/BackingManager.sol @@ -309,7 +309,16 @@ contract BackingManagerP1 is TradingP1, IBackingManager { rToken.setBasketsNeeded(basketsHeldBottom); } - // === Governance Setters === + // === Governance === + + /// Forcibly settle a trade, losing all value + /// Should only be called in case of censorship + /// @param trade The trade address itself + /// @custom:governance + function forceSettleTrade(ITrade trade) public override(TradingP1, ITrading) { + super.forceSettleTrade(trade); // enforces governance only + delete tokensOut[trade.sell()]; + } /// @custom:governance function setTradingDelay(uint48 val) public governance { diff --git a/contracts/p1/mixins/Trading.sol b/contracts/p1/mixins/Trading.sol index fbc845bb15..0798cb8cdc 100644 --- a/contracts/p1/mixins/Trading.sol +++ b/contracts/p1/mixins/Trading.sol @@ -140,7 +140,20 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl emit TradeStarted(trade, sell, req.buy.erc20(), req.sellAmount, req.minBuyAmount); } - // === Setters === + // === Governance === + + /// Forcibly settle a trade, losing all value + /// Should only be called in case of censorship + /// @param trade The trade address itself + /// @custom:governance + function forceSettleTrade(ITrade trade) public virtual governance { + // should not call any ERC20 functions, in case bricked + + IERC20Metadata sell = trade.sell(); + delete trades[sell]; + tradesOpen--; + emit TradeSettled(trade, sell, trade.buy(), 0, 0); + } /// @custom:governance function setMaxTradeSlippage(uint192 val) public governance { diff --git a/test/scenario/BadERC20.test.ts b/test/scenario/BadERC20.test.ts index e837428d43..690a686754 100644 --- a/test/scenario/BadERC20.test.ts +++ b/test/scenario/BadERC20.test.ts @@ -6,7 +6,7 @@ import { BigNumber } from 'ethers' import { ethers } from 'hardhat' import { IConfig } from '../../common/configuration' import { TradeKind } from '../../common/constants' -import { bn, divCeil, fp } from '../../common/numbers' +import { bn, fp } from '../../common/numbers' import { BadERC20, ERC20Mock, @@ -65,30 +65,6 @@ describe(`Bad ERC20 - P${IMPLEMENTATION}`, () => { let rsrTrader: TestIRevenueTrader let basketHandler: TestIBasketHandler - // Computes the minBuyAmt for a sellAmt at two prices - // sellPrice + buyPrice should not be the low and high estimates, but rather the oracle prices - const toMinBuyAmt = ( - sellAmt: BigNumber, - sellPrice: BigNumber, - buyPrice: BigNumber, - oracleError: BigNumber, - maxTradeSlippage: BigNumber - ): BigNumber => { - // do all muls first so we don't round unnecessarily - // a = loss due to max trade slippage - // b = loss due to selling token at the low price - // c = loss due to buying token at the high price - // mirrors the math from TradeLib ~L:57 - - const lowSellPrice = sellPrice.sub(sellPrice.mul(oracleError).div(fp('1'))) - const highBuyPrice = buyPrice.add(buyPrice.mul(oracleError).div(fp('1'))) - const product = sellAmt - .mul(fp('1').sub(maxTradeSlippage)) // (a) - .mul(lowSellPrice) // (b) - - return divCeil(divCeil(product, highBuyPrice), fp('1')) // (c) - } - beforeEach(async () => { ;[owner, addr1, addr2] = await ethers.getSigners() let erc20s: ERC20Mock[] @@ -325,6 +301,37 @@ describe(`Bad ERC20 - P${IMPLEMENTATION}`, () => { await backingManager.rebalance(TradeKind.BATCH_AUCTION) }) + it('should be able to force settle if an ERC20 breaks mid-auction', async () => { + await setOraclePrice(collateral0.address, bn('1e7')) // default + await collateral0.refresh() + await advanceTime(DELAY_UNTIL_DEFAULT.toString()) + await expect(basketHandler.refreshBasket()) + .to.emit(basketHandler, 'BasketSet') + .withArgs(2, [backupToken.address], [fp('1')], false) + + // Advance time post warmup period - SOUND just regained + await advanceTime(Number(config.warmupPeriod) + 1) + + await token0.setCensored(backingManager.address, false) + await backingManager.rebalance(TradeKind.BATCH_AUCTION) + + // Censor Trade while auction open + const t = await getTrade(backingManager, token0.address) + await token0.setCensored(t.address, true) + + // Advance to end of auction + await advanceTime(Number(config.dutchAuctionLength) + 1) + await expect(backingManager.settleTrade(token0.address)).to.be.revertedWith('censored') + + // Force settle + await backingManager.connect(owner).forceSettleTrade(t.address) + expect(await backingManager.tradesOpen()).to.equal(0) + + // Should be able to continue + await backingManager.rebalance(TradeKind.BATCH_AUCTION) + expect(await backingManager.tradesOpen()).to.equal(1) + }) + it('should keep collateral working', async () => { await collateral0.refresh() await collateral0.price() From 2d5a5999e9ea0c92cf74d9b8a53640f1bec54dfa Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 10:14:14 -0400 Subject: [PATCH 08/14] make EasyAuction address a detail of the GnosisTrade plugin (#1192) --- common/configuration.ts | 6 +- contracts/interfaces/IBroker.sol | 4 - contracts/interfaces/IDeployer.sol | 2 - contracts/p0/Broker.sol | 14 +-- contracts/p0/Deployer.sol | 3 +- contracts/p1/Broker.sol | 18 +-- contracts/p1/Deployer.sol | 6 +- contracts/plugins/mocks/InvalidBrokerMock.sol | 4 - .../plugins/mocks/upgrades/DeployerV2.sol | 3 +- .../{mocks => trading}/EasyAuction.sol | 3 + contracts/plugins/trading/GnosisTrade.sol | 10 +- .../{mocks => trading}/vendor/EasyAuction.sol | 2 + scripts/deployment/common.ts | 1 - .../phase1-core/0_setup_deployments.ts | 11 +- .../phase1-core/2_deploy_implementations.ts | 4 +- .../phase1-core/5_deploy_deployer.ts | 1 - scripts/deployment/utils.ts | 17 ++- tasks/deployment/deploy-easyauction.ts | 62 ++++++++++ .../mock/deploy-mock-easyauction.ts | 55 --------- tasks/index.ts | 2 +- test/Broker.test.ts | 111 +++++------------- test/Deployer.test.ts | 7 +- test/Main.test.ts | 3 +- test/Revenues.test.ts | 4 +- test/Upgradeability.test.ts | 6 +- test/fixtures.ts | 4 +- test/integration/EasyAuction.test.ts | 62 ++++------ test/integration/UpgradeToR4.test.ts | 11 +- .../UpgradeToR4WithRegistries.test.ts | 10 +- test/integration/fixtures.ts | 11 +- test/integration/fork-block-numbers.ts | 2 +- .../plugins/individual-collateral/fixtures.ts | 6 +- test/utils/trades.ts | 6 +- 33 files changed, 182 insertions(+), 289 deletions(-) rename contracts/plugins/{mocks => trading}/EasyAuction.sol (99%) rename contracts/plugins/{mocks => trading}/vendor/EasyAuction.sol (99%) create mode 100644 tasks/deployment/deploy-easyauction.ts delete mode 100644 tasks/deployment/mock/deploy-mock-easyauction.ts diff --git a/common/configuration.ts b/common/configuration.ts index 928d4b96ac..b93e8b90d5 100644 --- a/common/configuration.ts +++ b/common/configuration.ts @@ -157,7 +157,6 @@ interface INetworkConfig { COMPTROLLER?: string FLUX_FINANCE_COMPTROLLER?: string GNOSIS_EASY_AUCTION?: string - EASY_AUCTION_OWNER?: string MORPHO_AAVE_CONTROLLER?: string MORPHO_REWARDS_DISTRIBUTOR?: string MORPHO_AAVE_LENS?: string @@ -295,8 +294,7 @@ export const networkConfig: { [key: string]: INetworkConfig } = { AAVE_DATA_PROVIDER: '0x057835Ad21a177dbdd3090bB1CAE03EaCF78Fc6d', FLUX_FINANCE_COMPTROLLER: '0x95Af143a021DF745bc78e845b54591C53a8B3A51', COMPTROLLER: '0x3d9819210A31b4961b30EF54bE2aeD79B9c9Cd3B', - GNOSIS_EASY_AUCTION: '0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101', - EASY_AUCTION_OWNER: '0x0da0c3e52c977ed3cbc641ff02dd271c3ed55afe', + GNOSIS_EASY_AUCTION: '0x462302752B63Ee7c207459112CA8D38498Fb54f2', // our deployment MORPHO_AAVE_LENS: '0x507fA343d0A90786d86C7cd885f5C49263A91FF4', MORPHO_AAVE_CONTROLLER: '0x777777c9898D384F785Ee44Acfe945efDFf5f3E0', MORPHO_REWARDS_DISTRIBUTOR: '0x3b14e5c73e0a56d607a8688098326fd4b4292135', @@ -396,7 +394,7 @@ export const networkConfig: { [key: string]: INetworkConfig } = { AAVE_DATA_PROVIDER: '0x057835Ad21a177dbdd3090bB1CAE03EaCF78Fc6d', FLUX_FINANCE_COMPTROLLER: '0x95Af143a021DF745bc78e845b54591C53a8B3A51', COMPTROLLER: '0x3d9819210A31b4961b30EF54bE2aeD79B9c9Cd3B', - GNOSIS_EASY_AUCTION: '0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101', + GNOSIS_EASY_AUCTION: '0x462302752B63Ee7c207459112CA8D38498Fb54f2', // our deployment MORPHO_AAVE_LENS: '0x507fA343d0A90786d86C7cd885f5C49263A91FF4', MORPHO_AAVE_CONTROLLER: '0x777777c9898D384F785Ee44Acfe945efDFf5f3E0', MORPHO_REWARDS_DISTRIBUTOR: '0x3b14e5c73e0a56d607a8688098326fd4b4292135', diff --git a/contracts/interfaces/IBroker.sol b/contracts/interfaces/IBroker.sol index 369d47ac18..efc506ae09 100644 --- a/contracts/interfaces/IBroker.sol +++ b/contracts/interfaces/IBroker.sol @@ -34,7 +34,6 @@ struct TradeRequest { * the continued proper functioning of trading platforms. */ interface IBroker is IComponent { - event GnosisSet(IGnosis oldVal, IGnosis newVal); event BatchTradeImplementationSet(ITrade oldVal, ITrade newVal); event DutchTradeImplementationSet(ITrade oldVal, ITrade newVal); event BatchAuctionLengthSet(uint48 oldVal, uint48 newVal); @@ -45,7 +44,6 @@ interface IBroker is IComponent { // Initialization function init( IMain main_, - IGnosis gnosis_, ITrade batchTradeImplemention_, uint48 batchAuctionLength_, ITrade dutchTradeImplemention_, @@ -86,8 +84,6 @@ interface TestIBroker is IBroker { function dutchAuctionLength() external view returns (uint48); - function setGnosis(IGnosis newGnosis) external; - function setBatchTradeImplementation(ITrade newTradeImplementation) external; function setBatchAuctionLength(uint48 newAuctionLength) external; diff --git a/contracts/interfaces/IDeployer.sol b/contracts/interfaces/IDeployer.sol index 149394fc39..04246ccaa2 100644 --- a/contracts/interfaces/IDeployer.sol +++ b/contracts/interfaces/IDeployer.sol @@ -123,8 +123,6 @@ interface TestIDeployer is IDeployer { function rsr() external view returns (IERC20Metadata); - function gnosis() external view returns (IGnosis); - function rsrAsset() external view returns (IAsset); function implementations() external view returns (Implementations memory); diff --git a/contracts/p0/Broker.sol b/contracts/p0/Broker.sol index fffc341f16..902d1b0ed3 100644 --- a/contracts/p0/Broker.sol +++ b/contracts/p0/Broker.sol @@ -30,8 +30,6 @@ contract BrokerP0 is ComponentP0, IBroker { ITrade public batchTradeImplementation; ITrade public dutchTradeImplementation; - IGnosis public gnosis; - mapping(address => bool) private trades; uint48 public batchAuctionLength; // {s} the length of a Gnosis EasyAuction @@ -43,14 +41,12 @@ contract BrokerP0 is ComponentP0, IBroker { function init( IMain main_, - IGnosis gnosis_, ITrade batchTradeImplementation_, // Added for Interface compatibility with P1 uint48 batchAuctionLength_, ITrade dutchTradeImplementation_, // Added for Interface compatibility with P1 uint48 dutchAuctionLength_ ) public initializer { __Component_init(main_); - setGnosis(gnosis_); setBatchTradeImplementation(batchTradeImplementation_); setBatchAuctionLength(batchAuctionLength_); setDutchTradeImplementation(dutchTradeImplementation_); @@ -133,14 +129,6 @@ contract BrokerP0 is ComponentP0, IBroker { // === Setters === - /// @custom:governance - function setGnosis(IGnosis newGnosis) public governance { - require(address(newGnosis) != address(0), "invalid Gnosis address"); - - emit GnosisSet(gnosis, newGnosis); - gnosis = newGnosis; - } - /// @custom:governance function setBatchTradeImplementation(ITrade newTradeImplementation) public governance { require( @@ -220,7 +208,7 @@ contract BrokerP0 is ComponentP0, IBroker { req.sellAmount ); - trade.init(this, caller, gnosis, batchAuctionLength, req); + trade.init(this, caller, batchAuctionLength, req); return trade; } diff --git a/contracts/p0/Deployer.sol b/contracts/p0/Deployer.sol index f812830044..f2445d763c 100644 --- a/contracts/p0/Deployer.sol +++ b/contracts/p0/Deployer.sol @@ -119,8 +119,7 @@ contract DeployerP0 is IDeployer, Versioned { main.broker().init( main, - gnosis, - ITrade(address(new GnosisTrade())), + ITrade(address(new GnosisTrade(gnosis))), params.batchAuctionLength, ITrade(address(new DutchTrade())), params.dutchAuctionLength diff --git a/contracts/p1/Broker.sol b/contracts/p1/Broker.sol index 001f8419cd..6f925957c7 100644 --- a/contracts/p1/Broker.sol +++ b/contracts/p1/Broker.sol @@ -35,8 +35,9 @@ contract BrokerP1 is ComponentP1, IBroker { // The Batch Auction Trade contract to clone on openTrade(). Governance parameter. ITrade public batchTradeImplementation; - // The Gnosis contract to init batch auction trades with. Governance parameter. - IGnosis public gnosis; + /// @custom:oz-renamed-from gnosis + // Deprecated in 4.0.0 + IGnosis public gnosis_DEPRECATED; /// @custom:oz-renamed-from auctionLength // {s} the length of a Gnosis EasyAuction. Governance parameter. @@ -72,7 +73,6 @@ contract BrokerP1 is ComponentP1, IBroker { // effects: initial parameters are set function init( IMain main_, - IGnosis gnosis_, ITrade batchTradeImplementation_, uint48 batchAuctionLength_, ITrade dutchTradeImplementation_, @@ -81,8 +81,6 @@ contract BrokerP1 is ComponentP1, IBroker { __Component_init(main_); cacheComponents(); - setGnosis(gnosis_); - require( address(batchTradeImplementation_) != address(0), "invalid batchTradeImplementation address" @@ -174,14 +172,6 @@ contract BrokerP1 is ComponentP1, IBroker { // === Setters === - /// @custom:governance - function setGnosis(IGnosis newGnosis) public governance { - require(address(newGnosis) != address(0), "invalid Gnosis address"); - - emit GnosisSet(gnosis, newGnosis); - gnosis = newGnosis; - } - /// @custom:main function setBatchTradeImplementation(ITrade newTradeImplementation) public onlyMain { require( @@ -261,7 +251,7 @@ contract BrokerP1 is ComponentP1, IBroker { address(trade), req.sellAmount ); - trade.init(this, caller, gnosis, batchAuctionLength, req); + trade.init(this, caller, batchAuctionLength, req); return trade; } diff --git a/contracts/p1/Deployer.sol b/contracts/p1/Deployer.sol index 073c7c19e4..927d8a8a66 100644 --- a/contracts/p1/Deployer.sol +++ b/contracts/p1/Deployer.sol @@ -20,6 +20,7 @@ import "../plugins/assets/Asset.sol"; import "../plugins/assets/RTokenAsset.sol"; import "./Main.sol"; import "../libraries/String.sol"; +import "../plugins/trading/GnosisTrade.sol"; /** * @title DeployerP1 @@ -31,7 +32,6 @@ contract DeployerP1 is IDeployer, Versioned { string public constant ENS = "reserveprotocol.eth"; IERC20Metadata public immutable rsr; - IGnosis public immutable gnosis; IAsset public immutable rsrAsset; // Implementation contracts for Upgradeability @@ -41,13 +41,11 @@ contract DeployerP1 is IDeployer, Versioned { // effects: post, all contract-state values are set constructor( IERC20Metadata rsr_, - IGnosis gnosis_, IAsset rsrAsset_, Implementations memory implementations_ ) { require( address(rsr_) != address(0) && - address(gnosis_) != address(0) && address(rsrAsset_) != address(0) && address(implementations_.main) != address(0) && address(implementations_.trading.gnosisTrade) != address(0) && @@ -66,7 +64,6 @@ contract DeployerP1 is IDeployer, Versioned { ); rsr = rsr_; - gnosis = gnosis_; rsrAsset = rsrAsset_; _implementations = implementations_; } @@ -216,7 +213,6 @@ contract DeployerP1 is IDeployer, Versioned { components.broker.init( main, - gnosis, _implementations.trading.gnosisTrade, params.batchAuctionLength, _implementations.trading.dutchTrade, diff --git a/contracts/plugins/mocks/InvalidBrokerMock.sol b/contracts/plugins/mocks/InvalidBrokerMock.sol index 191df5136f..d1a4112637 100644 --- a/contracts/plugins/mocks/InvalidBrokerMock.sol +++ b/contracts/plugins/mocks/InvalidBrokerMock.sol @@ -15,8 +15,6 @@ contract InvalidBrokerMock is ComponentP0, IBroker { using EnumerableSet for EnumerableSet.AddressSet; using SafeERC20 for IERC20Metadata; - IGnosis public gnosis; - mapping(address => bool) private trades; uint48 public batchAuctionLength; // {s} the length of a batch auction @@ -28,14 +26,12 @@ contract InvalidBrokerMock is ComponentP0, IBroker { function init( IMain main_, - IGnosis gnosis_, ITrade, uint48 batchAuctionLength_, ITrade, uint48 dutchAuctionLength_ ) public initializer { __Component_init(main_); - gnosis = gnosis_; batchAuctionLength = batchAuctionLength_; dutchAuctionLength = dutchAuctionLength_; } diff --git a/contracts/plugins/mocks/upgrades/DeployerV2.sol b/contracts/plugins/mocks/upgrades/DeployerV2.sol index 2269ff86ca..4d6881bab2 100644 --- a/contracts/plugins/mocks/upgrades/DeployerV2.sol +++ b/contracts/plugins/mocks/upgrades/DeployerV2.sol @@ -9,10 +9,9 @@ contract DeployerP1V2 is DeployerP1 { constructor( IERC20Metadata rsr_, - IGnosis gnosis_, IAsset rsrAsset_, Implementations memory implementations_ - ) DeployerP1(rsr_, gnosis_, rsrAsset_, implementations_) {} + ) DeployerP1(rsr_, rsrAsset_, implementations_) {} function setNewValue(uint256 newValue_) external { newValue = newValue_; diff --git a/contracts/plugins/mocks/EasyAuction.sol b/contracts/plugins/trading/EasyAuction.sol similarity index 99% rename from contracts/plugins/mocks/EasyAuction.sol rename to contracts/plugins/trading/EasyAuction.sol index 8acf1a9ec9..3f6f8f22a0 100644 --- a/contracts/plugins/mocks/EasyAuction.sol +++ b/contracts/plugins/trading/EasyAuction.sol @@ -5,6 +5,9 @@ import "./vendor/EasyAuction.sol"; // ==== From https://etherscan.io/address/0x0b7ffc1f4ad541a4ed16b40d8c37f0929158d101 ==== +// solhint-disable + +// Used in production since 3.4.0 contract EasyAuction is Ownable { using SafeERC20 for IERC20; using SafeMath for uint64; diff --git a/contracts/plugins/trading/GnosisTrade.sol b/contracts/plugins/trading/GnosisTrade.sol index 082287d917..c9385a1dc2 100644 --- a/contracts/plugins/trading/GnosisTrade.sol +++ b/contracts/plugins/trading/GnosisTrade.sol @@ -34,12 +34,14 @@ contract GnosisTrade is ITrade, Versioned { // raw "/" for compile-time const uint192 public constant DEFAULT_MIN_BID = FIX_ONE / 100; // {tok} + IGnosis public immutable gnosis; // Gnosis Auction contract + // ==== status: This contract's state-machine state. See TradeStatus enum, above TradeStatus public status; // ==== The rest of contract state is all parameters that are immutable after init() // == Metadata - IGnosis public gnosis; // Gnosis Auction contract + IGnosis public gnosis_DEPRECATED; // made immutable in 4.0.0; left in for testing compatibility uint256 public auctionId; // The Gnosis Auction ID returned by gnosis.initiateAuction() IBroker public broker; // The Broker that cloned this contract into existence @@ -66,7 +68,9 @@ contract GnosisTrade is ITrade, Versioned { status = end; } - constructor() { + constructor(IGnosis _gnosis) { + require(address(_gnosis) != address(0), "gnosis address zero"); + gnosis = _gnosis; status = TradeStatus.CLOSED; } @@ -87,7 +91,6 @@ contract GnosisTrade is ITrade, Versioned { function init( IBroker broker_, address origin_, - IGnosis gnosis_, uint48 batchAuctionLength, TradeRequest calldata req ) external stateTransition(TradeStatus.NOT_STARTED, TradeStatus.OPEN) { @@ -105,7 +108,6 @@ contract GnosisTrade is ITrade, Versioned { broker = broker_; origin = origin_; - gnosis = gnosis_; endTime = uint48(block.timestamp) + batchAuctionLength; // D27{qBuyTok/qSellTok} diff --git a/contracts/plugins/mocks/vendor/EasyAuction.sol b/contracts/plugins/trading/vendor/EasyAuction.sol similarity index 99% rename from contracts/plugins/mocks/vendor/EasyAuction.sol rename to contracts/plugins/trading/vendor/EasyAuction.sol index a0b843948a..6961e3c1e5 100644 --- a/contracts/plugins/mocks/vendor/EasyAuction.sol +++ b/contracts/plugins/trading/vendor/EasyAuction.sol @@ -3,6 +3,8 @@ pragma solidity 0.6.12; // ==== From https://etherscan.io/address/0x0b7ffc1f4ad541a4ed16b40d8c37f0929158d101#code ==== +// solhint-disable + library AllowListVerifierHelper { /// @dev Value returned by a call to `isAllowed` if the check /// was successful. The value is defined as: diff --git a/scripts/deployment/common.ts b/scripts/deployment/common.ts index 1ed7958a16..a1f3393fd5 100644 --- a/scripts/deployment/common.ts +++ b/scripts/deployment/common.ts @@ -6,7 +6,6 @@ import { ITokens, IComponents, IImplementations, IPools } from '../../common/con export interface IPrerequisites { RSR: string RSR_FEED: string - GNOSIS_EASY_AUCTION: string } export interface IFacets { diff --git a/scripts/deployment/phase1-core/0_setup_deployments.ts b/scripts/deployment/phase1-core/0_setup_deployments.ts index f2c4666e54..f34073097e 100644 --- a/scripts/deployment/phase1-core/0_setup_deployments.ts +++ b/scripts/deployment/phase1-core/0_setup_deployments.ts @@ -39,19 +39,11 @@ async function main() { throw new Error(`RSR Feed contract not found in network ${hre.network.name}`) } - // Get Gnosis EasyAuction Address - const gnosisAddr = networkConfig[chainId].GNOSIS_EASY_AUCTION - if (!gnosisAddr) { - throw new Error(`Missing address for GNOSIS_EASY_AUCTION in network ${hre.network.name}`) - } else if (!(await isValidContract(hre, gnosisAddr))) { - throw new Error(`GNOSIS_EASY_AUCTION contract not found in network ${hre.network.name}`) - } // ********************* Output Configuration****************************** const deployments: IDeployments = { prerequisites: { RSR: rsrAddr, RSR_FEED: rsrFeedAddr, - GNOSIS_EASY_AUCTION: gnosisAddr, }, tradingLib: '', facade: '', @@ -59,6 +51,8 @@ async function main() { actFacet: '', readFacet: '', maxIssuableFacet: '', + backingBufferFacet: '', + revenueFacet: '', }, facadeWriteLib: '', basketLib: '', @@ -90,7 +84,6 @@ async function main() { console.log(`Deployment file created for ${hre.network.name} (${chainId}): RSR: ${rsrAddr} RSR FEED: ${rsrFeedAddr} - GNOSIS_EASY_AUCTION: ${gnosisAddr} Deployment file: ${deploymentFilename}`) } diff --git a/scripts/deployment/phase1-core/2_deploy_implementations.ts b/scripts/deployment/phase1-core/2_deploy_implementations.ts index b8a5098d66..e373fcee1c 100644 --- a/scripts/deployment/phase1-core/2_deploy_implementations.ts +++ b/scripts/deployment/phase1-core/2_deploy_implementations.ts @@ -129,7 +129,9 @@ async function main() { // ******************** Deploy GnosisTrade ********************************/ const GnosisTradeImplFactory = await ethers.getContractFactory('GnosisTrade') - gnosisTradeImpl = await GnosisTradeImplFactory.connect(burner).deploy() + gnosisTradeImpl = ( + await GnosisTradeImplFactory.connect(burner).deploy(networkConfig[chainId].GNOSIS_EASY_AUCTION!) + ) await gnosisTradeImpl.deployed() // Write temporary deployments file diff --git a/scripts/deployment/phase1-core/5_deploy_deployer.ts b/scripts/deployment/phase1-core/5_deploy_deployer.ts index 63448e2539..640c9d7b24 100644 --- a/scripts/deployment/phase1-core/5_deploy_deployer.ts +++ b/scripts/deployment/phase1-core/5_deploy_deployer.ts @@ -40,7 +40,6 @@ async function main() { deployer = ( await DeployerFactory.connect(burner).deploy( deployments.prerequisites.RSR, - deployments.prerequisites.GNOSIS_EASY_AUCTION, deployments.rsrAsset, deployments.implementations ) diff --git a/scripts/deployment/utils.ts b/scripts/deployment/utils.ts index 07f238d13c..35099e83ab 100644 --- a/scripts/deployment/utils.ts +++ b/scripts/deployment/utils.ts @@ -19,18 +19,12 @@ export const combinedError = (x: BigNumber, y: BigNumber): BigNumber => { export const validatePrerequisites = async (deployments: IDeployments) => { // Check prerequisites properly defined - if ( - !deployments.prerequisites.GNOSIS_EASY_AUCTION || - !deployments.prerequisites.RSR || - !deployments.prerequisites.RSR_FEED - ) { + if (!deployments.prerequisites.RSR || !deployments.prerequisites.RSR_FEED) { throw new Error(`Missing pre-requisite addresses in network ${hre.network.name}`) } else if (!(await isValidContract(hre, deployments.prerequisites.RSR))) { throw new Error(`RSR contract not found in network ${hre.network.name}`) } else if (!(await isValidContract(hre, deployments.prerequisites.RSR_FEED))) { throw new Error(`RSR_FEED contract not found in network ${hre.network.name}`) - } else if (!(await isValidContract(hre, deployments.prerequisites.GNOSIS_EASY_AUCTION))) { - throw new Error(`GNOSIS_EASY_AUCTION contract not found in network ${hre.network.name}`) } } @@ -178,11 +172,16 @@ export const getEmptyDeployment = (): IDeployments => { prerequisites: { RSR: '', RSR_FEED: '', - GNOSIS_EASY_AUCTION: '', }, tradingLib: '', basketLib: '', - facets: { actFacet: '', readFacet: '', maxIssuableFacet: '' }, + facets: { + actFacet: '', + readFacet: '', + maxIssuableFacet: '', + backingBufferFacet: '', + revenueFacet: '', + }, facade: '', facadeWriteLib: '', facadeWrite: '', diff --git a/tasks/deployment/deploy-easyauction.ts b/tasks/deployment/deploy-easyauction.ts new file mode 100644 index 0000000000..58ff78930e --- /dev/null +++ b/tasks/deployment/deploy-easyauction.ts @@ -0,0 +1,62 @@ +import { getChainId } from '../../common/blockchain-utils' +import { task, types } from 'hardhat/config' +import { EasyAuction } from '../../typechain' +import { ZERO_ADDRESS } from '../../common/constants' + +task('deploy-easyauction', 'Deploys a mock Easy Auction contract') + .addOptionalParam('noOutput', 'Suppress output', false, types.boolean) + .setAction(async (params, hre) => { + const [deployer] = await hre.ethers.getSigners() + + const chainId = await getChainId(hre) + + if (!params.noOutput) { + console.log( + `Deploying EasyAuction to ${hre.network.name} (${chainId}) with burner account ${deployer.address}` + ) + } + + const easyAuction = ( + await (await hre.ethers.getContractFactory('EasyAuction')).connect(deployer).deploy() + ) + await easyAuction.deployed() + + if (!params.noOutput) { + console.log( + `Deployed EasyAuction to ${hre.network.name} (${chainId}): ${easyAuction.address}` + ) + } + + // Hand away owner + await (await easyAuction.renounceOwnership()).wait() + if ((await easyAuction.owner()) != ZERO_ADDRESS) { + throw new Error('EasyAuction not renounced') + } + + if (!params.noOutput) { + console.log(`Renounced ownership`) + console.log('sleeping 12s') + } + + // Sleep to ensure API is in sync with chain + await new Promise((r) => setTimeout(r, 12000)) // 12s + + if (!params.noOutput) { + console.log('verifying') + } + + // /** ******************** Verify EasyAuction ****************************************/ + console.time('Verifying EasyAuction') + await hre.run('verify:verify', { + address: easyAuction.address, + constructorArguments: [], + contract: 'contracts/plugins/mocks/EasyAuction.sol:EasyAuction', + }) + console.timeEnd('Verifying EasyAuction') + + if (!params.noOutput) { + console.log('verified') + } + + return { feed: easyAuction.address } + }) diff --git a/tasks/deployment/mock/deploy-mock-easyauction.ts b/tasks/deployment/mock/deploy-mock-easyauction.ts deleted file mode 100644 index 0936b02979..0000000000 --- a/tasks/deployment/mock/deploy-mock-easyauction.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { getChainId } from '../../../common/blockchain-utils' -import { task, types } from 'hardhat/config' -import { EasyAuction } from '../../../typechain' - -task('deploy-mock-easyauction', 'Deploys a mock Easy Auction contract') - .addOptionalParam('noOutput', 'Suppress output', false, types.boolean) - .setAction(async (params, hre) => { - const [deployer] = await hre.ethers.getSigners() - - const chainId = await getChainId(hre) - - if (!params.noOutput) { - console.log( - `Deploying EasyAuction to ${hre.network.name} (${chainId}) with burner account ${deployer.address}` - ) - } - - const easyAuction = ( - await (await hre.ethers.getContractFactory('EasyAuction')).connect(deployer).deploy() - ) - await easyAuction.deployed() - - if (!params.noOutput) { - console.log( - `Deployed EasyAuction to ${hre.network.name} (${chainId}): ${easyAuction.address}` - ) - } - - // // Uncomment to verify - // if (!params.noOutput) { - // console.log('sleeping 30s') - // } - - // // Sleep to ensure API is in sync with chain - // await new Promise((r) => setTimeout(r, 30000)) // 30s - - // if (!params.noOutput) { - // console.log('verifying') - // } - - // // /** ******************** Verify EasyAuction ****************************************/ - // console.time('Verifying EasyAuction') - // await hre.run('verify:verify', { - // address: easyAuction.address, - // constructorArguments: [], - // contract: 'contracts/plugins/mocks/EasyAuction.sol:EasyAuction', - // }) - // console.timeEnd('Verifying EasyAuction') - - // if (!params.noOutput) { - // console.log('verified') - // } - - return { feed: easyAuction.address } - }) diff --git a/tasks/index.ts b/tasks/index.ts index ae59550e14..c4c0e13c4a 100644 --- a/tasks/index.ts +++ b/tasks/index.ts @@ -14,7 +14,7 @@ import './deployment/mock/deploy-mock-oracle' import './deployment/mock/deploy-mock-usdc' import './deployment/mock/deploy-mock-aave' import './deployment/mock/deploy-mock-wbtc' -import './deployment/mock/deploy-mock-easyauction' +import './deployment/deploy-easyauction' import './deployment/create-deployer-registry' import './deployment/create-curve-oracle-factory' import './deployment/deploy-facade-monitor' diff --git a/test/Broker.test.ts b/test/Broker.test.ts index 58d0ec73db..0f1ee660da 100644 --- a/test/Broker.test.ts +++ b/test/Broker.test.ts @@ -56,7 +56,7 @@ import { getLatestBlockTimestamp, setNextBlockTimestamp, } from './utils/time' -import { ITradeRequest, disableBatchTrade, disableDutchTrade, getTrade } from './utils/trades' +import { ITradeRequest, disableBatchTrade, disableDutchTrade } from './utils/trades' import { useEnv } from '#/utils/env' import { parseUnits } from 'ethers/lib/utils' @@ -136,7 +136,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { describe('Deployment', () => { it('Should setup Broker correctly', async () => { - expect(await broker.gnosis()).to.equal(gnosis.address) expect(await broker.batchAuctionLength()).to.equal(config.batchAuctionLength) expect(await broker.batchTradeDisabled()).to.equal(false) expect(await broker.dutchTradeDisabled(token0.address)).to.equal(false) @@ -158,65 +157,22 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { } await expect( - newBroker.init(main.address, ZERO_ADDRESS, ZERO_ADDRESS, bn('100'), ZERO_ADDRESS, bn('100')) - ).to.be.revertedWith('invalid Gnosis address') - await expect( - newBroker.init( - main.address, - gnosis.address, - ZERO_ADDRESS, - bn('1000'), - ZERO_ADDRESS, - bn('1000') - ) + newBroker.init(main.address, ZERO_ADDRESS, bn('1000'), ZERO_ADDRESS, bn('1000')) ).to.be.revertedWith('invalid batchTradeImplementation address') await expect( - newBroker.init( - main.address, - gnosis.address, - ONE_ADDRESS, - bn('1000'), - ZERO_ADDRESS, - bn('1000') - ) + newBroker.init(main.address, ONE_ADDRESS, bn('1000'), ZERO_ADDRESS, bn('1000')) ).to.be.revertedWith('invalid dutchTradeImplementation address') }) }) describe('Configuration/State', () => { - it('Should allow to update Gnosis if Owner and perform validations', async () => { - // Check existing value - expect(await broker.gnosis()).to.equal(gnosis.address) - - // If not owner cannot update - await expect(broker.connect(other).setGnosis(mock.address)).to.be.revertedWith( - 'governance only' - ) - - // Check value did not change - expect(await broker.gnosis()).to.equal(gnosis.address) - - // Attempt to update with Owner but zero address - not allowed - await expect(broker.connect(owner).setGnosis(ZERO_ADDRESS)).to.be.revertedWith( - 'invalid Gnosis address' - ) - - // Update with owner - await expect(broker.connect(owner).setGnosis(mock.address)) - .to.emit(broker, 'GnosisSet') - .withArgs(gnosis.address, mock.address) - - // Check value was updated - expect(await broker.gnosis()).to.equal(mock.address) - }) - it('Should allow to update BatchTrade Implementation if Owner and perform validations', async () => { const upgraderAddr = IMPLEMENTATION == Implementation.P1 ? main.address : owner.address const errorMsg = IMPLEMENTATION == Implementation.P1 ? 'main only' : 'governance only' // Create a Trade const TradeFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade') - const tradeImpl: GnosisTrade = await TradeFactory.deploy() + const tradeImpl: GnosisTrade = await TradeFactory.deploy(gnosis.address) // Update to a trade implementation to use as baseline for tests await whileImpersonating(upgraderAddr, async (upgSigner) => { @@ -570,19 +526,38 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { describe('Trades', () => { context('GnosisTrade', () => { const amount = fp('100.0') + let reentrantGnosis: GnosisMockReentrant + let tradeWithReentrantGnosis: GnosisTrade let trade: GnosisTrade beforeEach(async () => { // Create a Trade const TradeFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade') - trade = await TradeFactory.deploy() - + trade = await TradeFactory.deploy(gnosis.address) await setStorageAt(trade.address, 0, 0) + // Create a Trade with reentrant Gnosis + const GnosisReentrantFactory: ContractFactory = await ethers.getContractFactory( + 'GnosisMockReentrant' + ) + reentrantGnosis = await GnosisReentrantFactory.deploy() + tradeWithReentrantGnosis = await TradeFactory.deploy(reentrantGnosis.address) + await setStorageAt(tradeWithReentrantGnosis.address, 0, 0) + // Check state expect(await trade.status()).to.equal(TradeStatus.NOT_STARTED) expect(await trade.canSettle()).to.equal(false) + expect(await tradeWithReentrantGnosis.status()).to.equal(TradeStatus.NOT_STARTED) + expect(await tradeWithReentrantGnosis.canSettle()).to.equal(false) + }) + + it('Should not allow deployment with zero address gnosis', async () => { + const GnosisTradeFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade') + await expect(GnosisTradeFactory.deploy(ZERO_ADDRESS)).to.be.revertedWith( + 'gnosis address zero' + ) }) + it('Should initialize GnosisTrade correctly - only once', async () => { // Initialize trade - simulate from backingManager const tradeRequest: ITradeRequest = { @@ -598,7 +573,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -625,7 +599,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( await trade.broker(), await trade.origin(), - await trade.gnosis(), await broker.batchAuctionLength(), tradeRequest ) @@ -648,7 +621,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -671,13 +643,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { }) it('Should protect against reentrancy when initializing GnosisTrade', async () => { - // Create a Reetrant Gnosis - const GnosisReentrantFactory: ContractFactory = await ethers.getContractFactory( - 'GnosisMockReentrant' - ) - const reentrantGnosis: GnosisMockReentrant = ( - await GnosisReentrantFactory.deploy() - ) await reentrantGnosis.setReenterOnInit(true) // Initialize trade - simulate from backingManager @@ -689,12 +654,11 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { } // Fund trade and initialize with reentrant Gnosis - await token0.connect(owner).mint(trade.address, amount) + await token0.connect(owner).mint(tradeWithReentrantGnosis.address, amount) await expect( - trade.init( + tradeWithReentrantGnosis.init( broker.address, backingManager.address, - reentrantGnosis.address, config.batchAuctionLength, tradeRequest ) @@ -719,7 +683,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -734,7 +697,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -751,7 +713,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -772,7 +733,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -803,7 +763,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -926,13 +885,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { }) it('Should protect against reentrancy when settling GnosisTrade', async () => { - // Create a Reetrant Gnosis - const GnosisReentrantFactory: ContractFactory = await ethers.getContractFactory( - 'GnosisMockReentrant' - ) - const reentrantGnosis: GnosisMockReentrant = ( - await GnosisReentrantFactory.deploy() - ) await reentrantGnosis.setReenterOnInit(false) await reentrantGnosis.setReenterOnSettle(true) @@ -945,12 +897,11 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { } // Fund trade and initialize - await token0.connect(owner).mint(trade.address, amount) + await token0.connect(owner).mint(tradeWithReentrantGnosis.address, amount) await expect( - trade.init( + tradeWithReentrantGnosis.init( broker.address, backingManager.address, - reentrantGnosis.address, config.batchAuctionLength, tradeRequest ) @@ -980,7 +931,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -1054,7 +1004,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { trade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -1652,7 +1601,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { newTrade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) @@ -1667,7 +1615,6 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { await newTrade.init( broker.address, backingManager.address, - gnosis.address, config.batchAuctionLength, tradeRequest ) diff --git a/test/Deployer.test.ts b/test/Deployer.test.ts index 8711bcdc67..e75ca59e7b 100644 --- a/test/Deployer.test.ts +++ b/test/Deployer.test.ts @@ -80,7 +80,7 @@ describe(`DeployerP${IMPLEMENTATION} contract #fast`, () => { return await DeployerFactory.deploy(rsr, gnosis, rsrAsset) } else if (IMPLEMENTATION == Implementation.P1) { const DeployerFactory: ContractFactory = await ethers.getContractFactory('DeployerP1') - return await DeployerFactory.deploy(rsr, gnosis, rsrAsset, implementations) + return await DeployerFactory.deploy(rsr, rsrAsset, implementations) } else { throw new Error('PROTO_IMPL must be set to either `0` or `1`') } @@ -164,10 +164,6 @@ describe(`DeployerP${IMPLEMENTATION} contract #fast`, () => { deployNewDeployer(rsr.address, gnosis.address, ZERO_ADDRESS, implementations) ).to.be.revertedWith('invalid address') - await expect( - deployNewDeployer(rsr.address, ZERO_ADDRESS, rsrAsset.address, implementations) - ).to.be.revertedWith('invalid address') - await expect( deployNewDeployer(ZERO_ADDRESS, gnosis.address, rsrAsset.address, implementations) ).to.be.revertedWith('invalid address') @@ -216,7 +212,6 @@ describe(`DeployerP${IMPLEMENTATION} contract #fast`, () => { it('Should setup values correctly', async () => { expect(await deployer.ENS()).to.equal('reserveprotocol.eth') expect(await deployer.rsr()).to.equal(rsr.address) - expect(await deployer.gnosis()).to.equal(gnosis.address) expect(await deployer.rsrAsset()).to.equal(rsrAsset.address) }) diff --git a/test/Main.test.ts b/test/Main.test.ts index 3d77877a26..ca1d7dfad7 100644 --- a/test/Main.test.ts +++ b/test/Main.test.ts @@ -420,13 +420,12 @@ describe(`MainP${IMPLEMENTATION} contract`, () => { // Attempt to reinitialize - Broker const GnosisTradeFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade') - const gnosisTrade: GnosisTrade = await GnosisTradeFactory.deploy() + const gnosisTrade: GnosisTrade = await GnosisTradeFactory.deploy(gnosis.address) const DutchTradeFactory: ContractFactory = await ethers.getContractFactory('DutchTrade') const dutchTrade: DutchTrade = await DutchTradeFactory.deploy() await expect( broker.init( main.address, - gnosis.address, gnosisTrade.address, config.batchAuctionLength, dutchTrade.address, diff --git a/test/Revenues.test.ts b/test/Revenues.test.ts index fb984ce952..18d39ed3eb 100644 --- a/test/Revenues.test.ts +++ b/test/Revenues.test.ts @@ -151,8 +151,8 @@ describe(`Revenues - P${IMPLEMENTATION}`, () => { slot.replace(slot.slice(2, 14), '1'.padStart(12, '0')) ) } else { - const slot = await getStorageAt(broker.address, 56) - await setStorageAt(broker.address, 56, slot.replace(slot.slice(2, 42), '1'.padStart(40, '0'))) + const slot = await getStorageAt(broker.address, 55) + await setStorageAt(broker.address, 55, slot.replace(slot.slice(2, 42), '1'.padStart(40, '0'))) } expect(await broker.batchTradeDisabled()).to.equal(true) } diff --git a/test/Upgradeability.test.ts b/test/Upgradeability.test.ts index 8a610f7c4a..34c6af81ab 100644 --- a/test/Upgradeability.test.ts +++ b/test/Upgradeability.test.ts @@ -270,14 +270,13 @@ describeP1(`Upgradeability - P${IMPLEMENTATION}`, () => { }) it('Should deploy valid implementation - Broker / Trade', async () => { - const gnosisTrade: GnosisTrade = await GnosisTradeFactory.deploy() + const gnosisTrade: GnosisTrade = await GnosisTradeFactory.deploy(gnosis.address) const dutchTrade: DutchTrade = await DutchTradeFactory.deploy() const newBroker: BrokerP1 = await upgrades.deployProxy( BrokerFactory, [ main.address, - gnosis.address, gnosisTrade.address, config.batchAuctionLength, dutchTrade.address, @@ -290,7 +289,6 @@ describeP1(`Upgradeability - P${IMPLEMENTATION}`, () => { ) await newBroker.deployed() - expect(await newBroker.gnosis()).to.equal(gnosis.address) expect(await newBroker.batchAuctionLength()).to.equal(config.batchAuctionLength) expect(await newBroker.dutchAuctionLength()).to.equal(config.dutchAuctionLength) expect(await newBroker.batchTradeDisabled()).to.equal(false) @@ -702,7 +700,6 @@ describeP1(`Upgradeability - P${IMPLEMENTATION}`, () => { expect(brokerV2.address).to.equal(broker.address) // Check state is preserved - expect(await brokerV2.gnosis()).to.equal(gnosis.address) expect(await brokerV2.batchAuctionLength()).to.equal(config.batchAuctionLength) expect(await brokerV2.batchTradeDisabled()).to.equal(false) expect(await brokerV2.dutchTradeDisabled(rToken.address)).to.equal(false) @@ -934,7 +931,6 @@ describeP1(`Upgradeability - P${IMPLEMENTATION}`, () => { const DeployerV2Factory = await ethers.getContractFactory('DeployerP1V2') deployerV2 = await DeployerV2Factory.deploy( rsr.address, - gnosis.address, rsrAsset.address, implementationsV2 ) diff --git a/test/fixtures.ts b/test/fixtures.ts index a2a93b8c15..cf2dc04d66 100644 --- a/test/fixtures.ts +++ b/test/fixtures.ts @@ -579,7 +579,7 @@ const makeDefaultFixture = async (setBasket: boolean): Promise = const furnaceImpl: FurnaceP1 = await FurnaceImplFactory.deploy() const GnosisTradeImplFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade') - const gnosisTrade: GnosisTrade = await GnosisTradeImplFactory.deploy() + const gnosisTrade: GnosisTrade = await GnosisTradeImplFactory.deploy(gnosisAddr) const DutchTradeImplFactory: ContractFactory = await ethers.getContractFactory('DutchTrade') const dutchTrade: DutchTrade = await DutchTradeImplFactory.deploy() @@ -616,7 +616,7 @@ const makeDefaultFixture = async (setBasket: boolean): Promise = const DeployerFactory: ContractFactory = await ethers.getContractFactory('DeployerP1') deployer = ( - await DeployerFactory.deploy(rsr.address, gnosisAddr, rsrAsset.address, implementations) + await DeployerFactory.deploy(rsr.address, rsrAsset.address, implementations) ) } diff --git a/test/integration/EasyAuction.test.ts b/test/integration/EasyAuction.test.ts index 92520911b4..a8b32f6141 100644 --- a/test/integration/EasyAuction.test.ts +++ b/test/integration/EasyAuction.test.ts @@ -4,6 +4,9 @@ import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers' import { expect } from 'chai' import { BigNumber } from 'ethers' import hre, { ethers } from 'hardhat' +import { setStorageAt } from '@nomicfoundation/hardhat-network-helpers' +import forkBlockNumber from './fork-block-numbers' +import { resetFork } from '#/utils/chain' import { Collateral, IMPLEMENTATION, @@ -16,7 +19,7 @@ import { } from '../fixtures' import { bn, fp, shortString, divCeil } from '../../common/numbers' import { expectEvents } from '../../common/events' -import { IConfig, networkConfig } from '../../common/configuration' +import { IConfig } from '../../common/configuration' import { BN_SCALE_FACTOR, CollateralStatus, @@ -32,8 +35,6 @@ import { import { advanceTime, getLatestBlockTimestamp } from '../utils/time' import { expectTrade, getAuctionId, getTrade } from '../utils/trades' import { setOraclePrice } from '../utils/oracles' -import { getChainId } from '../../common/blockchain-utils' -import { whileImpersonating } from '../utils/impersonation' import { expectRTokenPrice } from '../utils/oracles' import { withinTolerance } from '../utils/matchers' import { cartesianProduct } from '../utils/cases' @@ -85,6 +86,10 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function let token1: ERC20Mock let rTokenAsset: RTokenAsset + before(async () => { + await resetFork(hre, forkBlockNumber.default) + }) + beforeEach(async () => { ;[owner, addr1, addr2] = await ethers.getSigners() @@ -110,6 +115,9 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function token0 = erc20s[collateral.indexOf(basket[0])] token1 = erc20s[collateral.indexOf(basket[1])] collateral0 = collateral[0] + + // Put owner in charge of EasyAuction + await setStorageAt(easyAuction.address, 0, owner.address) }) context('RSR -> token0', function () { @@ -435,13 +443,8 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function }) it('full volume -- with fees', async () => { - const chainId = await getChainId(hre) - const easyAuctionOwner = networkConfig[chainId].EASY_AUCTION_OWNER || '' - // Apply 0.1% fee - await whileImpersonating(easyAuctionOwner, async (auctionOwner) => { - await easyAuction.connect(auctionOwner).setFeeParameters(1, easyAuctionOwner) - }) + await easyAuction.connect(owner).setFeeParameters(1, owner.address) const adjSellAmt = sellAmt.mul(1000).div(1001) const bidAmt = buyAmt @@ -653,16 +656,11 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function }) it('should handle fees in EasyAuction correctly', async () => { - const chainId = await getChainId(hre) - const easyAuctionOwner = networkConfig[chainId].EASY_AUCTION_OWNER || '' - // No fees yet transferred to Easy auction owner - expect(await token0.balanceOf(easyAuctionOwner)).to.equal(0) + expect(await token0.balanceOf(owner.address)).to.equal(0) // Set fees in easy auction to 1% - await whileImpersonating(easyAuctionOwner, async (auctionOwner) => { - await easyAuction.connect(auctionOwner).setFeeParameters(10, easyAuctionOwner) - }) + await easyAuction.connect(owner).setFeeParameters(10, owner.address) // Calculate values const feeDenominator = await easyAuction.FEE_DENOMINATOR() @@ -728,7 +726,7 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function }, ]) - expect(await token0.balanceOf(easyAuctionOwner)).to.be.closeTo(feeAmt, 1) // account for rounding + expect(await token0.balanceOf(owner.address)).to.be.closeTo(feeAmt, 1) // account for rounding }) }) @@ -829,7 +827,7 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function // Deployments const main = await MainFactory.deploy() const broker = await BrokerFactory.deploy() - const gnosisTradeImpl = await GnosisTradeFactory.deploy() + const gnosisTradeImpl = await GnosisTradeFactory.deploy(easyAuction.address) const dutchTradeImpl = await DutchTradeFactory.deploy() await main.init( { @@ -854,7 +852,6 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function await main.connect(owner).unpauseIssuance() await broker.init( main.address, - easyAuction.address, gnosisTradeImpl.address, config.batchAuctionLength, dutchTradeImpl.address, @@ -1000,21 +997,7 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function // This set the broker to false since it was one token short. // This test is to make sure that the broker is not disabled in this case. - const resetFork = async () => { - await hre.network.provider.request({ - method: 'hardhat_reset', - params: [ - { - forking: { - jsonRpcUrl: process.env.MAINNET_RPC_URL, - blockNumber: 16813289, - }, - }, - ], - }) - } - - await resetFork() + await resetFork(hre, 16813289) const backingManager = await ethers.getContractAt( 'BackingManagerP1', @@ -1026,14 +1009,19 @@ describeFork(`Gnosis EasyAuction Mainnet Forking - P${IMPLEMENTATION}`, function true ) - await resetFork() + await resetFork(hre, 16813289) const gnosisTradeImpl = await ethers.getContractAt( 'GnosisTrade', '0xAc543Ee89A2238945f7D7Ad4d9Cf958721f9757c' ) - const gnosisTradeArtifact = await hre.artifacts.readArtifact('GnosisTrade') - await setCode(gnosisTradeImpl.address, gnosisTradeArtifact.deployedBytecode) + + const GnosisTradeFactory = await ethers.getContractFactory('GnosisTrade') + const gnosisTrade = await GnosisTradeFactory.deploy( + '0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101' // OG EasyAuction + ) + const bytecode = await ethers.provider.send('eth_getCode', [gnosisTrade.address]) + await setCode(gnosisTradeImpl.address, bytecode) await backingManager.settleTrade('0x39AA39c021dfbaE8faC545936693aC917d5E7563') expect(await broker.attach('0x90EB22A31b69C29C34162E0E9278cc0617aA2B50').disabled()).to.equal( diff --git a/test/integration/UpgradeToR4.test.ts b/test/integration/UpgradeToR4.test.ts index 3d6d24db09..66a0416e8c 100644 --- a/test/integration/UpgradeToR4.test.ts +++ b/test/integration/UpgradeToR4.test.ts @@ -1,13 +1,13 @@ import hre, { ethers } from 'hardhat' -import { reset } from '@nomicfoundation/hardhat-network-helpers' import { VersionRegistry } from '@typechain/VersionRegistry' import { expect } from 'chai' -import { forkRpcs } from '#/utils/fork' import { IImplementations } from '#/common/configuration' import { DeployerP1 } from '@typechain/DeployerP1' import { AssetPluginRegistry } from '@typechain/AssetPluginRegistry' import { whileImpersonating } from '#/utils/impersonation' import { DAOFeeRegistry } from '@typechain/DAOFeeRegistry' +import { resetFork } from '#/utils/chain' +import forkBlockNumber from './fork-block-numbers' interface RTokenParams { name: string @@ -40,9 +40,10 @@ describe('Upgrade from 3.4.0 to 4.0.0 (Mainnet Fork)', () => { let daoFeeRegistry: DAOFeeRegistry before(async () => { - await reset(forkRpcs.mainnet, 19991614) const [owner] = await ethers.getSigners() + await resetFork(hre, forkBlockNumber.default) + const TradingLibFactory = await ethers.getContractFactory('RecollateralizationLibP1') const BasketLibFactory = await ethers.getContractFactory('BasketLibP1') const tradingLib = await TradingLibFactory.deploy() @@ -84,7 +85,8 @@ describe('Upgrade from 3.4.0 to 4.0.0 (Mainnet Fork)', () => { stRSR: (await StRSRFactory.deploy()).address, }, trading: { - gnosisTrade: (await GnosisTradeFactory.deploy()).address, + gnosisTrade: (await GnosisTradeFactory.deploy('0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101')) + .address, dutchTrade: (await DutchTradeFactory.deploy()).address, }, } @@ -92,7 +94,6 @@ describe('Upgrade from 3.4.0 to 4.0.0 (Mainnet Fork)', () => { const DeployerFactory = await ethers.getContractFactory('DeployerP1') deployer = await DeployerFactory.deploy( '0x320623b8E4fF03373931769A31Fc52A4E78B5d70', - '0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101', '0x591529f039Ba48C3bEAc5090e30ceDDcb41D0EaA', implementations ) diff --git a/test/integration/UpgradeToR4WithRegistries.test.ts b/test/integration/UpgradeToR4WithRegistries.test.ts index 632a4d37ee..61648d3215 100644 --- a/test/integration/UpgradeToR4WithRegistries.test.ts +++ b/test/integration/UpgradeToR4WithRegistries.test.ts @@ -103,7 +103,9 @@ describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => stRSR: (await StRSRFactory.deploy()).address, }, trading: { - gnosisTrade: (await GnosisTradeFactory.deploy()).address, + gnosisTrade: ( + await GnosisTradeFactory.deploy('0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101') + ).address, dutchTrade: (await DutchTradeFactory.deploy()).address, }, } @@ -111,7 +113,6 @@ describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => const DeployerFactory = await ethers.getContractFactory('DeployerP1') const deployerR4 = await DeployerFactory.deploy( '0x320623b8E4fF03373931769A31Fc52A4E78B5d70', - '0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101', '0x591529f039Ba48C3bEAc5090e30ceDDcb41D0EaA', implementationsR4 ) @@ -156,7 +157,9 @@ describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => stRSR: (await StRSRFactory.deploy()).address, }, trading: { - gnosisTrade: (await GnosisTradeFactory.deploy()).address, + gnosisTrade: ( + await GnosisTradeFactory.deploy('0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101') + ).address, dutchTrade: (await DutchTradeFactory.deploy()).address, }, } @@ -164,7 +167,6 @@ describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => const DeployerFactory = await ethers.getContractFactory('DeployerP1V2') const deployerR2 = await DeployerFactory.deploy( '0x320623b8E4fF03373931769A31Fc52A4E78B5d70', - '0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101', '0x591529f039Ba48C3bEAc5090e30ceDDcb41D0EaA', implementationsR2 ) diff --git a/test/integration/fixtures.ts b/test/integration/fixtures.ts index cc0049d9b0..d41f5f2632 100644 --- a/test/integration/fixtures.ts +++ b/test/integration/fixtures.ts @@ -796,7 +796,9 @@ const makeDefaultFixture = async (setBasket: boolean): Promise = const furnaceImpl: FurnaceP1 = await FurnaceImplFactory.deploy() const GnosisTradeImplFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade') - const gnosisTrade: GnosisTrade = await GnosisTradeImplFactory.deploy() + const gnosisTrade: GnosisTrade = ( + await GnosisTradeImplFactory.deploy(easyAuction.address) + ) const DutchTradeImplFactory: ContractFactory = await ethers.getContractFactory('DutchTrade') const dutchTrade: DutchTrade = await DutchTradeImplFactory.deploy() @@ -833,12 +835,7 @@ const makeDefaultFixture = async (setBasket: boolean): Promise = const DeployerFactory: ContractFactory = await ethers.getContractFactory('DeployerP1') deployer = ( - await DeployerFactory.deploy( - rsr.address, - easyAuction.address, - rsrAsset.address, - implementations - ) + await DeployerFactory.deploy(rsr.address, rsrAsset.address, implementations) ) } diff --git a/test/integration/fork-block-numbers.ts b/test/integration/fork-block-numbers.ts index 70b563090c..fe2541150a 100644 --- a/test/integration/fork-block-numbers.ts +++ b/test/integration/fork-block-numbers.ts @@ -11,7 +11,7 @@ const forkBlockNumber = { 'mainnet-3.4.0': 20328530, // Ethereum // TODO add all the block numbers we fork from to benefit from caching - default: 19742528, // Ethereum + default: 20679946, // Ethereum } export default forkBlockNumber diff --git a/test/plugins/individual-collateral/fixtures.ts b/test/plugins/individual-collateral/fixtures.ts index e83b6bae25..454fdc36b2 100644 --- a/test/plugins/individual-collateral/fixtures.ts +++ b/test/plugins/individual-collateral/fixtures.ts @@ -183,7 +183,9 @@ export const getDefaultFixture = async function (salt: string) { const furnaceImpl: FurnaceP1 = await FurnaceImplFactory.deploy() const GnosisTradeImplFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade') - const gnosisTrade: GnosisTrade = await GnosisTradeImplFactory.deploy() + const gnosisTrade: GnosisTrade = ( + await GnosisTradeImplFactory.deploy(gnosis.address) + ) const DutchTradeImplFactory: ContractFactory = await ethers.getContractFactory('DutchTrade') const dutchTrade: DutchTrade = await DutchTradeImplFactory.deploy() @@ -216,7 +218,7 @@ export const getDefaultFixture = async function (salt: string) { } const DeployerFactory: ContractFactory = await ethers.getContractFactory('DeployerP1') deployer = ( - await DeployerFactory.deploy(rsr.address, gnosis.address, rsrAsset.address, implementations) + await DeployerFactory.deploy(rsr.address, rsrAsset.address, implementations) ) } diff --git a/test/utils/trades.ts b/test/utils/trades.ts index 99e0f4c22f..42e84bc640 100644 --- a/test/utils/trades.ts +++ b/test/utils/trades.ts @@ -126,14 +126,14 @@ export const disableBatchTrade = async (broker: TestIBroker) => { const slot = await getStorageAt(broker.address, 205) await setStorageAt(broker.address, 205, slot.replace(slot.slice(2, 14), '1'.padStart(12, '0'))) } else { - const slot = await getStorageAt(broker.address, 56) - await setStorageAt(broker.address, 56, slot.replace(slot.slice(2, 42), '1'.padStart(40, '0'))) + const slot = await getStorageAt(broker.address, 55) + await setStorageAt(broker.address, 55, slot.replace(slot.slice(2, 42), '1'.padStart(40, '0'))) } expect(await broker.batchTradeDisabled()).to.equal(true) } export const disableDutchTrade = async (broker: TestIBroker, erc20: string) => { - const mappingSlot = IMPLEMENTATION == Implementation.P1 ? bn('208') : bn('57') + const mappingSlot = IMPLEMENTATION == Implementation.P1 ? bn('208') : bn('56') const p = mappingSlot.toHexString().slice(2).padStart(64, '0') const key = erc20.slice(2).padStart(64, '0') const slot = ethers.utils.keccak256('0x' + key + p) From 75b0b014d9fb4648709f544b9ff5e88bb9ddebfa Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 10:19:23 -0400 Subject: [PATCH 09/14] prevent queueing of proposals from different eras (#1196) Co-authored-by: Akshat Mittal --- .solhintignore | 1 + contracts/plugins/governance/Governance.sol | 14 +- .../plugins/governance/vendor/Governor.sol | 781 ++++++++++++++++++ contracts/plugins/governance/vendor/README.md | 5 + .../extensions/GovernorCountingSimple.sol | 100 +++ .../vendor/extensions/GovernorSettings.sol | 110 +++ .../extensions/GovernorTimelockControl.sol | 186 +++++ .../vendor/extensions/GovernorVotes.sol | 55 ++ .../GovernorVotesQuorumFraction.sol | 132 +++ test/Governance.test.ts | 53 ++ 10 files changed, 1430 insertions(+), 7 deletions(-) create mode 100644 contracts/plugins/governance/vendor/Governor.sol create mode 100644 contracts/plugins/governance/vendor/README.md create mode 100644 contracts/plugins/governance/vendor/extensions/GovernorCountingSimple.sol create mode 100644 contracts/plugins/governance/vendor/extensions/GovernorSettings.sol create mode 100644 contracts/plugins/governance/vendor/extensions/GovernorTimelockControl.sol create mode 100644 contracts/plugins/governance/vendor/extensions/GovernorVotes.sol create mode 100644 contracts/plugins/governance/vendor/extensions/GovernorVotesQuorumFraction.sol diff --git a/.solhintignore b/.solhintignore index db227ef747..f8752aef16 100644 --- a/.solhintignore +++ b/.solhintignore @@ -20,3 +20,4 @@ contracts/vendor contracts/plugins/assets/compound/vendor contracts/plugins/assets/curve/crv contracts/plugins/assets/curve/cvx +contracts/plugins/governance/vendor diff --git a/contracts/plugins/governance/Governance.sol b/contracts/plugins/governance/Governance.sol index d4a9665823..12c100eaa4 100644 --- a/contracts/plugins/governance/Governance.sol +++ b/contracts/plugins/governance/Governance.sol @@ -1,13 +1,13 @@ // SPDX-License-Identifier: BlueOak-1.0.0 pragma solidity 0.8.19; -import "@openzeppelin/contracts/governance/Governor.sol"; -import "@openzeppelin/contracts/governance/extensions/GovernorCountingSimple.sol"; -import "@openzeppelin/contracts/governance/extensions/GovernorSettings.sol"; -import "@openzeppelin/contracts/governance/extensions/GovernorTimelockControl.sol"; -import "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; -import "@openzeppelin/contracts/governance/extensions/GovernorVotesQuorumFraction.sol"; import "../../interfaces/IStRSRVotes.sol"; +import "./vendor/extensions/GovernorCountingSimple.sol"; +import "./vendor/extensions/GovernorSettings.sol"; +import "./vendor/extensions/GovernorTimelockControl.sol"; +import "./vendor/extensions/GovernorVotes.sol"; +import "./vendor/extensions/GovernorVotesQuorumFraction.sol"; +import "./vendor/Governor.sol"; uint256 constant ONE_DAY = 86400; // {s} @@ -183,7 +183,7 @@ contract Governance is // === Private === function startedInSameEra(uint256 proposalId) private view returns (bool) { - uint256 startTimepoint = proposalSnapshot(proposalId); + uint256 startTimepoint = proposalProposedAt(proposalId); uint256 pastEra = IStRSRVotes(address(token)).getPastEra(startTimepoint); uint256 currentEra = IStRSRVotes(address(token)).currentEra(); return currentEra == pastEra; diff --git a/contracts/plugins/governance/vendor/Governor.sol b/contracts/plugins/governance/vendor/Governor.sol new file mode 100644 index 0000000000..6431652b55 --- /dev/null +++ b/contracts/plugins/governance/vendor/Governor.sol @@ -0,0 +1,781 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.1) (governance/Governor.sol) + +pragma solidity ^0.8.0; + +import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol"; +import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol"; +import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; +import "@openzeppelin/contracts/utils/introspection/ERC165.sol"; +import "@openzeppelin/contracts/utils/math/SafeCast.sol"; +import "@openzeppelin/contracts/utils/structs/DoubleEndedQueue.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; +import "@openzeppelin/contracts/utils/Context.sol"; +import "@openzeppelin/contracts/governance/IGovernor.sol"; + +// This contract is based on OZ Governor 4.9.6 +// The only change is to expose an additional proposalProposedAt() view function + +/** + * @dev Core of the governance system, designed to be extended though various modules. + * + * This contract is abstract and requires several functions to be implemented in various modules: + * + * - A counting module must implement {quorum}, {_quorumReached}, {_voteSucceeded} and {_countVote} + * - A voting module must implement {_getVotes} + * - Additionally, {votingPeriod} must also be implemented + * + * _Available since v4.3._ + */ +abstract contract Governor is + Context, + ERC165, + EIP712, + IGovernor, + IERC721Receiver, + IERC1155Receiver +{ + using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque; + + bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)"); + bytes32 public constant EXTENDED_BALLOT_TYPEHASH = + keccak256("ExtendedBallot(uint256 proposalId,uint8 support,string reason,bytes params)"); + + // solhint-disable var-name-mixedcase + struct ProposalCore { + // --- start retyped from Timers.BlockNumber at offset 0x00 --- + uint64 voteStart; + address proposer; + bytes4 __gap_unused0; + // --- start retyped from Timers.BlockNumber at offset 0x20 --- + uint64 voteEnd; + uint64 proposedAt; + bytes16 __gap_unused1; + // --- Remaining fields starting at offset 0x40 --------------- + bool executed; + bool canceled; + } + // solhint-enable var-name-mixedcase + + string private _name; + + /// @custom:oz-retyped-from mapping(uint256 => Governor.ProposalCore) + mapping(uint256 => ProposalCore) private _proposals; + + // This queue keeps track of the governor operating on itself. Calls to functions protected by the + // {onlyGovernance} modifier needs to be whitelisted in this queue. Whitelisting is set in {_beforeExecute}, + // consumed by the {onlyGovernance} modifier and eventually reset in {_afterExecute}. This ensures that the + // execution of {onlyGovernance} protected calls can only be achieved through successful proposals. + DoubleEndedQueue.Bytes32Deque private _governanceCall; + + /** + * @dev Restricts a function so it can only be executed through governance proposals. For example, governance + * parameter setters in {GovernorSettings} are protected using this modifier. + * + * The governance executing address may be different from the Governor's own address, for example it could be a + * timelock. This can be customized by modules by overriding {_executor}. The executor is only able to invoke these + * functions during the execution of the governor's {execute} function, and not under any other circumstances. Thus, + * for example, additional timelock proposers are not able to change governance parameters without going through the + * governance protocol (since v4.6). + */ + modifier onlyGovernance() { + require(_msgSender() == _executor(), "Governor: onlyGovernance"); + if (_executor() != address(this)) { + bytes32 msgDataHash = keccak256(_msgData()); + // loop until popping the expected operation - throw if deque is empty (operation not authorized) + while (_governanceCall.popFront() != msgDataHash) {} + } + _; + } + + /** + * @dev Sets the value for {name} and {version} + */ + constructor(string memory name_) EIP712(name_, version()) { + _name = name_; + } + + /** + * @dev Function to receive ETH that will be handled by the governor (disabled if executor is a third party contract) + */ + receive() external payable virtual { + require(_executor() == address(this), "Governor: must send to executor"); + } + + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(IERC165, ERC165) + returns (bool) + { + bytes4 governorCancelId = this.cancel.selector ^ this.proposalProposer.selector; + + bytes4 governorParamsId = this.castVoteWithReasonAndParams.selector ^ + this.castVoteWithReasonAndParamsBySig.selector ^ + this.getVotesWithParams.selector; + + // The original interface id in v4.3. + bytes4 governor43Id = type(IGovernor).interfaceId ^ + type(IERC6372).interfaceId ^ + governorCancelId ^ + governorParamsId; + + // An updated interface id in v4.6, with params added. + bytes4 governor46Id = type(IGovernor).interfaceId ^ + type(IERC6372).interfaceId ^ + governorCancelId; + + // For the updated interface id in v4.9, we use governorCancelId directly. + + return + interfaceId == governor43Id || + interfaceId == governor46Id || + interfaceId == governorCancelId || + interfaceId == type(IERC1155Receiver).interfaceId || + super.supportsInterface(interfaceId); + } + + /** + * @dev See {IGovernor-name}. + */ + function name() public view virtual override returns (string memory) { + return _name; + } + + /** + * @dev See {IGovernor-version}. + */ + function version() public view virtual override returns (string memory) { + return "1"; + } + + /** + * @dev See {IGovernor-hashProposal}. + * + * The proposal id is produced by hashing the ABI encoded `targets` array, the `values` array, the `calldatas` array + * and the descriptionHash (bytes32 which itself is the keccak256 hash of the description string). This proposal id + * can be produced from the proposal data which is part of the {ProposalCreated} event. It can even be computed in + * advance, before the proposal is submitted. + * + * Note that the chainId and the governor address are not part of the proposal id computation. Consequently, the + * same proposal (with same operation and same description) will have the same id if submitted on multiple governors + * across multiple networks. This also means that in order to execute the same operation twice (on the same + * governor) the proposer will have to change the description in order to avoid proposal id conflicts. + */ + function hashProposal( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public pure virtual override returns (uint256) { + return uint256(keccak256(abi.encode(targets, values, calldatas, descriptionHash))); + } + + /** + * @dev See {IGovernor-state}. + */ + function state(uint256 proposalId) public view virtual override returns (ProposalState) { + ProposalCore storage proposal = _proposals[proposalId]; + + if (proposal.executed) { + return ProposalState.Executed; + } + + if (proposal.canceled) { + return ProposalState.Canceled; + } + + uint256 snapshot = proposalSnapshot(proposalId); + + if (snapshot == 0) { + revert("Governor: unknown proposal id"); + } + + uint256 currentTimepoint = clock(); + + if (snapshot >= currentTimepoint) { + return ProposalState.Pending; + } + + uint256 deadline = proposalDeadline(proposalId); + + if (deadline >= currentTimepoint) { + return ProposalState.Active; + } + + if (_quorumReached(proposalId) && _voteSucceeded(proposalId)) { + return ProposalState.Succeeded; + } else { + return ProposalState.Defeated; + } + } + + /** + * @dev Part of the Governor Bravo's interface: _"The number of votes required in order for a voter to become a proposer"_. + */ + function proposalThreshold() public view virtual returns (uint256) { + return 0; + } + + function proposalProposedAt(uint256 proposalId) public view virtual returns (uint256) { + return _proposals[proposalId].proposedAt; + } + + /** + * @dev See {IGovernor-proposalSnapshot}. + */ + function proposalSnapshot(uint256 proposalId) public view virtual override returns (uint256) { + return _proposals[proposalId].voteStart; + } + + /** + * @dev See {IGovernor-proposalDeadline}. + */ + function proposalDeadline(uint256 proposalId) public view virtual override returns (uint256) { + return _proposals[proposalId].voteEnd; + } + + /** + * @dev Returns the account that created a given proposal. + */ + function proposalProposer(uint256 proposalId) public view virtual override returns (address) { + return _proposals[proposalId].proposer; + } + + /** + * @dev Amount of votes already cast passes the threshold limit. + */ + function _quorumReached(uint256 proposalId) internal view virtual returns (bool); + + /** + * @dev Is the proposal successful or not. + */ + function _voteSucceeded(uint256 proposalId) internal view virtual returns (bool); + + /** + * @dev Get the voting weight of `account` at a specific `timepoint`, for a vote as described by `params`. + */ + function _getVotes( + address account, + uint256 timepoint, + bytes memory params + ) internal view virtual returns (uint256); + + /** + * @dev Register a vote for `proposalId` by `account` with a given `support`, voting `weight` and voting `params`. + * + * Note: Support is generic and can represent various things depending on the voting system used. + */ + function _countVote( + uint256 proposalId, + address account, + uint8 support, + uint256 weight, + bytes memory params + ) internal virtual; + + /** + * @dev Default additional encoded parameters used by castVote methods that don't include them + * + * Note: Should be overridden by specific implementations to use an appropriate value, the + * meaning of the additional params, in the context of that implementation + */ + function _defaultParams() internal view virtual returns (bytes memory) { + return ""; + } + + /** + * @dev See {IGovernor-propose}. This function has opt-in frontrunning protection, described in {_isValidDescriptionForProposer}. + */ + function propose( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + string memory description + ) public virtual override returns (uint256) { + address proposer = _msgSender(); + require( + _isValidDescriptionForProposer(proposer, description), + "Governor: proposer restricted" + ); + + uint256 currentTimepoint = clock(); + require( + getVotes(proposer, currentTimepoint - 1) >= proposalThreshold(), + "Governor: proposer votes below proposal threshold" + ); + + uint256 proposalId = hashProposal( + targets, + values, + calldatas, + keccak256(bytes(description)) + ); + + require(targets.length == values.length, "Governor: invalid proposal length"); + require(targets.length == calldatas.length, "Governor: invalid proposal length"); + require(targets.length > 0, "Governor: empty proposal"); + require(_proposals[proposalId].voteStart == 0, "Governor: proposal already exists"); + + uint256 snapshot = currentTimepoint + votingDelay(); + uint256 deadline = snapshot + votingPeriod(); + + _proposals[proposalId] = ProposalCore({ + proposer: proposer, + proposedAt: SafeCast.toUint64(currentTimepoint), + voteStart: SafeCast.toUint64(snapshot), + voteEnd: SafeCast.toUint64(deadline), + executed: false, + canceled: false, + __gap_unused0: 0, + __gap_unused1: 0 + }); + + emit ProposalCreated( + proposalId, + proposer, + targets, + values, + new string[](targets.length), + calldatas, + snapshot, + deadline, + description + ); + + return proposalId; + } + + /** + * @dev See {IGovernor-execute}. + */ + function execute( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public payable virtual override returns (uint256) { + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); + + ProposalState currentState = state(proposalId); + require( + currentState == ProposalState.Succeeded || currentState == ProposalState.Queued, + "Governor: proposal not successful" + ); + _proposals[proposalId].executed = true; + + emit ProposalExecuted(proposalId); + + _beforeExecute(proposalId, targets, values, calldatas, descriptionHash); + _execute(proposalId, targets, values, calldatas, descriptionHash); + _afterExecute(proposalId, targets, values, calldatas, descriptionHash); + + return proposalId; + } + + /** + * @dev See {IGovernor-cancel}. + */ + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public virtual override returns (uint256) { + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); + require(state(proposalId) == ProposalState.Pending, "Governor: too late to cancel"); + require( + _msgSender() == _proposals[proposalId].proposer, + "Governor: only proposer can cancel" + ); + return _cancel(targets, values, calldatas, descriptionHash); + } + + /** + * @dev Internal execution mechanism. Can be overridden to implement different execution mechanism + */ + function _execute( + uint256, /* proposalId */ + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 /*descriptionHash*/ + ) internal virtual { + string memory errorMessage = "Governor: call reverted without message"; + for (uint256 i = 0; i < targets.length; ++i) { + (bool success, bytes memory returndata) = targets[i].call{ value: values[i] }( + calldatas[i] + ); + Address.verifyCallResult(success, returndata, errorMessage); + } + } + + /** + * @dev Hook before execution is triggered. + */ + function _beforeExecute( + uint256, /* proposalId */ + address[] memory targets, + uint256[] memory, /* values */ + bytes[] memory calldatas, + bytes32 /*descriptionHash*/ + ) internal virtual { + if (_executor() != address(this)) { + for (uint256 i = 0; i < targets.length; ++i) { + if (targets[i] == address(this)) { + _governanceCall.pushBack(keccak256(calldatas[i])); + } + } + } + } + + /** + * @dev Hook after execution is triggered. + */ + function _afterExecute( + uint256, /* proposalId */ + address[] memory, /* targets */ + uint256[] memory, /* values */ + bytes[] memory, /* calldatas */ + bytes32 /*descriptionHash*/ + ) internal virtual { + if (_executor() != address(this)) { + if (!_governanceCall.empty()) { + _governanceCall.clear(); + } + } + } + + /** + * @dev Internal cancel mechanism: locks up the proposal timer, preventing it from being re-submitted. Marks it as + * canceled to allow distinguishing it from executed proposals. + * + * Emits a {IGovernor-ProposalCanceled} event. + */ + function _cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) internal virtual returns (uint256) { + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); + + ProposalState currentState = state(proposalId); + + require( + currentState != ProposalState.Canceled && + currentState != ProposalState.Expired && + currentState != ProposalState.Executed, + "Governor: proposal not active" + ); + _proposals[proposalId].canceled = true; + + emit ProposalCanceled(proposalId); + + return proposalId; + } + + /** + * @dev See {IGovernor-getVotes}. + */ + function getVotes(address account, uint256 timepoint) + public + view + virtual + override + returns (uint256) + { + return _getVotes(account, timepoint, _defaultParams()); + } + + /** + * @dev See {IGovernor-getVotesWithParams}. + */ + function getVotesWithParams( + address account, + uint256 timepoint, + bytes memory params + ) public view virtual override returns (uint256) { + return _getVotes(account, timepoint, params); + } + + /** + * @dev See {IGovernor-castVote}. + */ + function castVote(uint256 proposalId, uint8 support) public virtual override returns (uint256) { + address voter = _msgSender(); + return _castVote(proposalId, voter, support, ""); + } + + /** + * @dev See {IGovernor-castVoteWithReason}. + */ + function castVoteWithReason( + uint256 proposalId, + uint8 support, + string calldata reason + ) public virtual override returns (uint256) { + address voter = _msgSender(); + return _castVote(proposalId, voter, support, reason); + } + + /** + * @dev See {IGovernor-castVoteWithReasonAndParams}. + */ + function castVoteWithReasonAndParams( + uint256 proposalId, + uint8 support, + string calldata reason, + bytes memory params + ) public virtual override returns (uint256) { + address voter = _msgSender(); + return _castVote(proposalId, voter, support, reason, params); + } + + /** + * @dev See {IGovernor-castVoteBySig}. + */ + function castVoteBySig( + uint256 proposalId, + uint8 support, + uint8 v, + bytes32 r, + bytes32 s + ) public virtual override returns (uint256) { + address voter = ECDSA.recover( + _hashTypedDataV4(keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support))), + v, + r, + s + ); + return _castVote(proposalId, voter, support, ""); + } + + /** + * @dev See {IGovernor-castVoteWithReasonAndParamsBySig}. + */ + function castVoteWithReasonAndParamsBySig( + uint256 proposalId, + uint8 support, + string calldata reason, + bytes memory params, + uint8 v, + bytes32 r, + bytes32 s + ) public virtual override returns (uint256) { + address voter = ECDSA.recover( + _hashTypedDataV4( + keccak256( + abi.encode( + EXTENDED_BALLOT_TYPEHASH, + proposalId, + support, + keccak256(bytes(reason)), + keccak256(params) + ) + ) + ), + v, + r, + s + ); + + return _castVote(proposalId, voter, support, reason, params); + } + + /** + * @dev Internal vote casting mechanism: Check that the vote is pending, that it has not been cast yet, retrieve + * voting weight using {IGovernor-getVotes} and call the {_countVote} internal function. Uses the _defaultParams(). + * + * Emits a {IGovernor-VoteCast} event. + */ + function _castVote( + uint256 proposalId, + address account, + uint8 support, + string memory reason + ) internal virtual returns (uint256) { + return _castVote(proposalId, account, support, reason, _defaultParams()); + } + + /** + * @dev Internal vote casting mechanism: Check that the vote is pending, that it has not been cast yet, retrieve + * voting weight using {IGovernor-getVotes} and call the {_countVote} internal function. + * + * Emits a {IGovernor-VoteCast} event. + */ + function _castVote( + uint256 proposalId, + address account, + uint8 support, + string memory reason, + bytes memory params + ) internal virtual returns (uint256) { + ProposalCore storage proposal = _proposals[proposalId]; + require(state(proposalId) == ProposalState.Active, "Governor: vote not currently active"); + + uint256 weight = _getVotes(account, proposal.voteStart, params); + _countVote(proposalId, account, support, weight, params); + + if (params.length == 0) { + emit VoteCast(account, proposalId, support, weight, reason); + } else { + emit VoteCastWithParams(account, proposalId, support, weight, reason, params); + } + + return weight; + } + + /** + * @dev Relays a transaction or function call to an arbitrary target. In cases where the governance executor + * is some contract other than the governor itself, like when using a timelock, this function can be invoked + * in a governance proposal to recover tokens or Ether that was sent to the governor contract by mistake. + * Note that if the executor is simply the governor itself, use of `relay` is redundant. + */ + function relay( + address target, + uint256 value, + bytes calldata data + ) external payable virtual onlyGovernance { + (bool success, bytes memory returndata) = target.call{ value: value }(data); + Address.verifyCallResult(success, returndata, "Governor: relay reverted without message"); + } + + /** + * @dev Address through which the governor executes action. Will be overloaded by module that execute actions + * through another contract such as a timelock. + */ + function _executor() internal view virtual returns (address) { + return address(this); + } + + /** + * @dev See {IERC721Receiver-onERC721Received}. + */ + function onERC721Received( + address, + address, + uint256, + bytes memory + ) public virtual override returns (bytes4) { + return this.onERC721Received.selector; + } + + /** + * @dev See {IERC1155Receiver-onERC1155Received}. + */ + function onERC1155Received( + address, + address, + uint256, + uint256, + bytes memory + ) public virtual override returns (bytes4) { + return this.onERC1155Received.selector; + } + + /** + * @dev See {IERC1155Receiver-onERC1155BatchReceived}. + */ + function onERC1155BatchReceived( + address, + address, + uint256[] memory, + uint256[] memory, + bytes memory + ) public virtual override returns (bytes4) { + return this.onERC1155BatchReceived.selector; + } + + /** + * @dev Check if the proposer is authorized to submit a proposal with the given description. + * + * If the proposal description ends with `#proposer=0x???`, where `0x???` is an address written as a hex string + * (case insensitive), then the submission of this proposal will only be authorized to said address. + * + * This is used for frontrunning protection. By adding this pattern at the end of their proposal, one can ensure + * that no other address can submit the same proposal. An attacker would have to either remove or change that part, + * which would result in a different proposal id. + * + * If the description does not match this pattern, it is unrestricted and anyone can submit it. This includes: + * - If the `0x???` part is not a valid hex string. + * - If the `0x???` part is a valid hex string, but does not contain exactly 40 hex digits. + * - If it ends with the expected suffix followed by newlines or other whitespace. + * - If it ends with some other similar suffix, e.g. `#other=abc`. + * - If it does not end with any such suffix. + */ + function _isValidDescriptionForProposer(address proposer, string memory description) + internal + view + virtual + returns (bool) + { + uint256 len = bytes(description).length; + + // Length is too short to contain a valid proposer suffix + if (len < 52) { + return true; + } + + // Extract what would be the `#proposer=0x` marker beginning the suffix + bytes12 marker; + assembly { + // - Start of the string contents in memory = description + 32 + // - First character of the marker = len - 52 + // - Length of "#proposer=0x0000000000000000000000000000000000000000" = 52 + // - We read the memory word starting at the first character of the marker: + // - (description + 32) + (len - 52) = description + (len - 20) + // - Note: Solidity will ignore anything past the first 12 bytes + marker := mload(add(description, sub(len, 20))) + } + + // If the marker is not found, there is no proposer suffix to check + if (marker != bytes12("#proposer=0x")) { + return true; + } + + // Parse the 40 characters following the marker as uint160 + uint160 recovered = 0; + for (uint256 i = len - 40; i < len; ++i) { + (bool isHex, uint8 value) = _tryHexToUint(bytes(description)[i]); + // If any of the characters is not a hex digit, ignore the suffix entirely + if (!isHex) { + return true; + } + recovered = (recovered << 4) | value; + } + + return recovered == uint160(proposer); + } + + /** + * @dev Try to parse a character from a string as a hex value. Returns `(true, value)` if the char is in + * `[0-9a-fA-F]` and `(false, 0)` otherwise. Value is guaranteed to be in the range `0 <= value < 16` + */ + function _tryHexToUint(bytes1 char) private pure returns (bool, uint8) { + uint8 c = uint8(char); + unchecked { + // Case 0-9 + if (47 < c && c < 58) { + return (true, c - 48); + } + // Case A-F + else if (64 < c && c < 71) { + return (true, c - 55); + } + // Case a-f + else if (96 < c && c < 103) { + return (true, c - 87); + } + // Else: not a hex char + else { + return (false, 0); + } + } + } +} diff --git a/contracts/plugins/governance/vendor/README.md b/contracts/plugins/governance/vendor/README.md new file mode 100644 index 0000000000..d14ab8127a --- /dev/null +++ b/contracts/plugins/governance/vendor/README.md @@ -0,0 +1,5 @@ +# OZ Governor Vendored Contracts + +This directory contains contracts that are vendored from OpenZeppelin 4.9.6. They have been lighly modified to track the proposedAt field in the ProposalCore struct of the Governor contract. + +All other modifications are cosmetic only (indentation; import order; etc). diff --git a/contracts/plugins/governance/vendor/extensions/GovernorCountingSimple.sol b/contracts/plugins/governance/vendor/extensions/GovernorCountingSimple.sol new file mode 100644 index 0000000000..b9517445a2 --- /dev/null +++ b/contracts/plugins/governance/vendor/extensions/GovernorCountingSimple.sol @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (governance/extensions/GovernorCountingSimple.sol) + +pragma solidity ^0.8.0; + +import "../Governor.sol"; + +/** + * @dev Extension of {Governor} for simple, 3 options, vote counting. + * + * _Available since v4.3._ + */ +abstract contract GovernorCountingSimple is Governor { + /** + * @dev Supported vote types. Matches Governor Bravo ordering. + */ + enum VoteType { + Against, + For, + Abstain + } + + struct ProposalVote { + uint256 againstVotes; + uint256 forVotes; + uint256 abstainVotes; + mapping(address => bool) hasVoted; + } + + mapping(uint256 => ProposalVote) private _proposalVotes; + + /** + * @dev See {IGovernor-COUNTING_MODE}. + */ + // solhint-disable-next-line func-name-mixedcase + function COUNTING_MODE() public pure virtual override returns (string memory) { + return "support=bravo&quorum=for,abstain"; + } + + /** + * @dev See {IGovernor-hasVoted}. + */ + function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) { + return _proposalVotes[proposalId].hasVoted[account]; + } + + /** + * @dev Accessor to the internal vote counts. + */ + function proposalVotes( + uint256 proposalId + ) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) { + ProposalVote storage proposalVote = _proposalVotes[proposalId]; + return (proposalVote.againstVotes, proposalVote.forVotes, proposalVote.abstainVotes); + } + + /** + * @dev See {Governor-_quorumReached}. + */ + function _quorumReached(uint256 proposalId) internal view virtual override returns (bool) { + ProposalVote storage proposalVote = _proposalVotes[proposalId]; + + return quorum(proposalSnapshot(proposalId)) <= proposalVote.forVotes + proposalVote.abstainVotes; + } + + /** + * @dev See {Governor-_voteSucceeded}. In this module, the forVotes must be strictly over the againstVotes. + */ + function _voteSucceeded(uint256 proposalId) internal view virtual override returns (bool) { + ProposalVote storage proposalVote = _proposalVotes[proposalId]; + + return proposalVote.forVotes > proposalVote.againstVotes; + } + + /** + * @dev See {Governor-_countVote}. In this module, the support follows the `VoteType` enum (from Governor Bravo). + */ + function _countVote( + uint256 proposalId, + address account, + uint8 support, + uint256 weight, + bytes memory // params + ) internal virtual override { + ProposalVote storage proposalVote = _proposalVotes[proposalId]; + + require(!proposalVote.hasVoted[account], "GovernorVotingSimple: vote already cast"); + proposalVote.hasVoted[account] = true; + + if (support == uint8(VoteType.Against)) { + proposalVote.againstVotes += weight; + } else if (support == uint8(VoteType.For)) { + proposalVote.forVotes += weight; + } else if (support == uint8(VoteType.Abstain)) { + proposalVote.abstainVotes += weight; + } else { + revert("GovernorVotingSimple: invalid value for enum VoteType"); + } + } +} diff --git a/contracts/plugins/governance/vendor/extensions/GovernorSettings.sol b/contracts/plugins/governance/vendor/extensions/GovernorSettings.sol new file mode 100644 index 0000000000..ec6a98300f --- /dev/null +++ b/contracts/plugins/governance/vendor/extensions/GovernorSettings.sol @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (governance/extensions/GovernorSettings.sol) + +pragma solidity ^0.8.0; + +import "../Governor.sol"; + +/** + * @dev Extension of {Governor} for settings updatable through governance. + * + * _Available since v4.4._ + */ +abstract contract GovernorSettings is Governor { + uint256 private _votingDelay; + uint256 private _votingPeriod; + uint256 private _proposalThreshold; + + event VotingDelaySet(uint256 oldVotingDelay, uint256 newVotingDelay); + event VotingPeriodSet(uint256 oldVotingPeriod, uint256 newVotingPeriod); + event ProposalThresholdSet(uint256 oldProposalThreshold, uint256 newProposalThreshold); + + /** + * @dev Initialize the governance parameters. + */ + constructor(uint256 initialVotingDelay, uint256 initialVotingPeriod, uint256 initialProposalThreshold) { + _setVotingDelay(initialVotingDelay); + _setVotingPeriod(initialVotingPeriod); + _setProposalThreshold(initialProposalThreshold); + } + + /** + * @dev See {IGovernor-votingDelay}. + */ + function votingDelay() public view virtual override returns (uint256) { + return _votingDelay; + } + + /** + * @dev See {IGovernor-votingPeriod}. + */ + function votingPeriod() public view virtual override returns (uint256) { + return _votingPeriod; + } + + /** + * @dev See {Governor-proposalThreshold}. + */ + function proposalThreshold() public view virtual override returns (uint256) { + return _proposalThreshold; + } + + /** + * @dev Update the voting delay. This operation can only be performed through a governance proposal. + * + * Emits a {VotingDelaySet} event. + */ + function setVotingDelay(uint256 newVotingDelay) public virtual onlyGovernance { + _setVotingDelay(newVotingDelay); + } + + /** + * @dev Update the voting period. This operation can only be performed through a governance proposal. + * + * Emits a {VotingPeriodSet} event. + */ + function setVotingPeriod(uint256 newVotingPeriod) public virtual onlyGovernance { + _setVotingPeriod(newVotingPeriod); + } + + /** + * @dev Update the proposal threshold. This operation can only be performed through a governance proposal. + * + * Emits a {ProposalThresholdSet} event. + */ + function setProposalThreshold(uint256 newProposalThreshold) public virtual onlyGovernance { + _setProposalThreshold(newProposalThreshold); + } + + /** + * @dev Internal setter for the voting delay. + * + * Emits a {VotingDelaySet} event. + */ + function _setVotingDelay(uint256 newVotingDelay) internal virtual { + emit VotingDelaySet(_votingDelay, newVotingDelay); + _votingDelay = newVotingDelay; + } + + /** + * @dev Internal setter for the voting period. + * + * Emits a {VotingPeriodSet} event. + */ + function _setVotingPeriod(uint256 newVotingPeriod) internal virtual { + // voting period must be at least one block long + require(newVotingPeriod > 0, "GovernorSettings: voting period too low"); + emit VotingPeriodSet(_votingPeriod, newVotingPeriod); + _votingPeriod = newVotingPeriod; + } + + /** + * @dev Internal setter for the proposal threshold. + * + * Emits a {ProposalThresholdSet} event. + */ + function _setProposalThreshold(uint256 newProposalThreshold) internal virtual { + emit ProposalThresholdSet(_proposalThreshold, newProposalThreshold); + _proposalThreshold = newProposalThreshold; + } +} diff --git a/contracts/plugins/governance/vendor/extensions/GovernorTimelockControl.sol b/contracts/plugins/governance/vendor/extensions/GovernorTimelockControl.sol new file mode 100644 index 0000000000..8985766bc0 --- /dev/null +++ b/contracts/plugins/governance/vendor/extensions/GovernorTimelockControl.sol @@ -0,0 +1,186 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (governance/extensions/GovernorTimelockControl.sol) + +pragma solidity ^0.8.0; + +import "@openzeppelin/contracts/governance/extensions/IGovernorTimelock.sol"; +import "@openzeppelin/contracts/governance/TimelockController.sol"; +import "../Governor.sol"; + +/** + * @dev Extension of {Governor} that binds the execution process to an instance of {TimelockController}. This adds a + * delay, enforced by the {TimelockController} to all successful proposal (in addition to the voting duration). The + * {Governor} needs the proposer (and ideally the executor) roles for the {Governor} to work properly. + * + * Using this model means the proposal will be operated by the {TimelockController} and not by the {Governor}. Thus, + * the assets and permissions must be attached to the {TimelockController}. Any asset sent to the {Governor} will be + * inaccessible. + * + * WARNING: Setting up the TimelockController to have additional proposers besides the governor is very risky, as it + * grants them powers that they must be trusted or known not to use: 1) {onlyGovernance} functions like {relay} are + * available to them through the timelock, and 2) approved governance proposals can be blocked by them, effectively + * executing a Denial of Service attack. This risk will be mitigated in a future release. + * + * _Available since v4.3._ + */ +abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { + TimelockController private _timelock; + mapping(uint256 => bytes32) private _timelockIds; + + /** + * @dev Emitted when the timelock controller used for proposal execution is modified. + */ + event TimelockChange(address oldTimelock, address newTimelock); + + /** + * @dev Set the timelock. + */ + constructor(TimelockController timelockAddress) { + _updateTimelock(timelockAddress); + } + + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(IERC165, Governor) + returns (bool) + { + return + interfaceId == type(IGovernorTimelock).interfaceId || + super.supportsInterface(interfaceId); + } + + /** + * @dev Overridden version of the {Governor-state} function with added support for the `Queued` state. + */ + function state(uint256 proposalId) + public + view + virtual + override(IGovernor, Governor) + returns (ProposalState) + { + ProposalState currentState = super.state(proposalId); + + if (currentState != ProposalState.Succeeded) { + return currentState; + } + + // core tracks execution, so we just have to check if successful proposal have been queued. + bytes32 queueid = _timelockIds[proposalId]; + if (queueid == bytes32(0)) { + return currentState; + } else if (_timelock.isOperationDone(queueid)) { + return ProposalState.Executed; + } else if (_timelock.isOperationPending(queueid)) { + return ProposalState.Queued; + } else { + return ProposalState.Canceled; + } + } + + /** + * @dev Public accessor to check the address of the timelock + */ + function timelock() public view virtual override returns (address) { + return address(_timelock); + } + + /** + * @dev Public accessor to check the eta of a queued proposal + */ + function proposalEta(uint256 proposalId) public view virtual override returns (uint256) { + uint256 eta = _timelock.getTimestamp(_timelockIds[proposalId]); + return eta == 1 ? 0 : eta; // _DONE_TIMESTAMP (1) should be replaced with a 0 value + } + + /** + * @dev Function to queue a proposal to the timelock. + */ + function queue( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) public virtual override returns (uint256) { + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); + + require(state(proposalId) == ProposalState.Succeeded, "Governor: proposal not successful"); + + uint256 delay = _timelock.getMinDelay(); + _timelockIds[proposalId] = _timelock.hashOperationBatch( + targets, + values, + calldatas, + 0, + descriptionHash + ); + _timelock.scheduleBatch(targets, values, calldatas, 0, descriptionHash, delay); + + emit ProposalQueued(proposalId, block.timestamp + delay); + + return proposalId; + } + + /** + * @dev Overridden execute function that run the already queued proposal through the timelock. + */ + function _execute( + uint256, /* proposalId */ + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) internal virtual override { + _timelock.executeBatch{ value: msg.value }(targets, values, calldatas, 0, descriptionHash); + } + + /** + * @dev Overridden version of the {Governor-_cancel} function to cancel the timelocked proposal if it as already + * been queued. + */ + // This function can reenter through the external call to the timelock, but we assume the timelock is trusted and + // well behaved (according to TimelockController) and this will not happen. + // slither-disable-next-line reentrancy-no-eth + function _cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) internal virtual override returns (uint256) { + uint256 proposalId = super._cancel(targets, values, calldatas, descriptionHash); + + if (_timelockIds[proposalId] != 0) { + _timelock.cancel(_timelockIds[proposalId]); + delete _timelockIds[proposalId]; + } + + return proposalId; + } + + /** + * @dev Address through which the governor executes action. In this case, the timelock. + */ + function _executor() internal view virtual override returns (address) { + return address(_timelock); + } + + /** + * @dev Public endpoint to update the underlying timelock instance. Restricted to the timelock itself, so updates + * must be proposed, scheduled, and executed through governance proposals. + * + * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals. + */ + function updateTimelock(TimelockController newTimelock) external virtual onlyGovernance { + _updateTimelock(newTimelock); + } + + function _updateTimelock(TimelockController newTimelock) private { + emit TimelockChange(address(_timelock), address(newTimelock)); + _timelock = newTimelock; + } +} diff --git a/contracts/plugins/governance/vendor/extensions/GovernorVotes.sol b/contracts/plugins/governance/vendor/extensions/GovernorVotes.sol new file mode 100644 index 0000000000..e2174787bd --- /dev/null +++ b/contracts/plugins/governance/vendor/extensions/GovernorVotes.sol @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (governance/extensions/GovernorVotes.sol) + +pragma solidity ^0.8.0; + +import "../Governor.sol"; +import "@openzeppelin/contracts/interfaces/IERC5805.sol"; + +/** + * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token, or since v4.5 an {ERC721Votes} token. + * + * _Available since v4.3._ + */ +abstract contract GovernorVotes is Governor { + IERC5805 public immutable token; + + constructor(IVotes tokenAddress) { + token = IERC5805(address(tokenAddress)); + } + + /** + * @dev Clock (as specified in EIP-6372) is set to match the token's clock. Fallback to block numbers if the token + * does not implement EIP-6372. + */ + function clock() public view virtual override returns (uint48) { + try token.clock() returns (uint48 timepoint) { + return timepoint; + } catch { + return SafeCast.toUint48(block.number); + } + } + + /** + * @dev Machine-readable description of the clock as specified in EIP-6372. + */ + // solhint-disable-next-line func-name-mixedcase + function CLOCK_MODE() public view virtual override returns (string memory) { + try token.CLOCK_MODE() returns (string memory clockmode) { + return clockmode; + } catch { + return "mode=blocknumber&from=default"; + } + } + + /** + * Read the voting weight from the token's built in snapshot mechanism (see {Governor-_getVotes}). + */ + function _getVotes( + address account, + uint256 timepoint, + bytes memory /*params*/ + ) internal view virtual override returns (uint256) { + return token.getPastVotes(account, timepoint); + } +} diff --git a/contracts/plugins/governance/vendor/extensions/GovernorVotesQuorumFraction.sol b/contracts/plugins/governance/vendor/extensions/GovernorVotesQuorumFraction.sol new file mode 100644 index 0000000000..b9bb3c69c3 --- /dev/null +++ b/contracts/plugins/governance/vendor/extensions/GovernorVotesQuorumFraction.sol @@ -0,0 +1,132 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (governance/extensions/GovernorVotesQuorumFraction.sol) + +pragma solidity ^0.8.0; + +import "@openzeppelin/contracts/utils/Checkpoints.sol"; +import "@openzeppelin/contracts/utils/math/SafeCast.sol"; +import "./GovernorVotes.sol"; + +/** + * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token and a quorum expressed as a + * fraction of the total supply. + * + * _Available since v4.3._ + */ +abstract contract GovernorVotesQuorumFraction is GovernorVotes { + using Checkpoints for Checkpoints.Trace224; + + uint256 private _quorumNumerator; // DEPRECATED in favor of _quorumNumeratorHistory + + /// @custom:oz-retyped-from Checkpoints.History + Checkpoints.Trace224 private _quorumNumeratorHistory; + + event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator); + + /** + * @dev Initialize quorum as a fraction of the token's total supply. + * + * The fraction is specified as `numerator / denominator`. By default the denominator is 100, so quorum is + * specified as a percent: a numerator of 10 corresponds to quorum being 10% of total supply. The denominator can be + * customized by overriding {quorumDenominator}. + */ + constructor(uint256 quorumNumeratorValue) { + _updateQuorumNumerator(quorumNumeratorValue); + } + + /** + * @dev Returns the current quorum numerator. See {quorumDenominator}. + */ + function quorumNumerator() public view virtual returns (uint256) { + return + _quorumNumeratorHistory._checkpoints.length == 0 + ? _quorumNumerator + : _quorumNumeratorHistory.latest(); + } + + /** + * @dev Returns the quorum numerator at a specific timepoint. See {quorumDenominator}. + */ + function quorumNumerator(uint256 timepoint) public view virtual returns (uint256) { + // If history is empty, fallback to old storage + uint256 length = _quorumNumeratorHistory._checkpoints.length; + if (length == 0) { + return _quorumNumerator; + } + + // Optimistic search, check the latest checkpoint + Checkpoints.Checkpoint224 memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; + if (latest._key <= timepoint) { + return latest._value; + } + + // Otherwise, do the binary search + return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint32(timepoint)); + } + + /** + * @dev Returns the quorum denominator. Defaults to 100, but may be overridden. + */ + function quorumDenominator() public view virtual returns (uint256) { + return 100; + } + + /** + * @dev Returns the quorum for a timepoint, in terms of number of votes: `supply * numerator / denominator`. + */ + function quorum(uint256 timepoint) public view virtual override returns (uint256) { + return + (token.getPastTotalSupply(timepoint) * quorumNumerator(timepoint)) / + quorumDenominator(); + } + + /** + * @dev Changes the quorum numerator. + * + * Emits a {QuorumNumeratorUpdated} event. + * + * Requirements: + * + * - Must be called through a governance proposal. + * - New numerator must be smaller or equal to the denominator. + */ + function updateQuorumNumerator(uint256 newQuorumNumerator) external virtual onlyGovernance { + _updateQuorumNumerator(newQuorumNumerator); + } + + /** + * @dev Changes the quorum numerator. + * + * Emits a {QuorumNumeratorUpdated} event. + * + * Requirements: + * + * - New numerator must be smaller or equal to the denominator. + */ + function _updateQuorumNumerator(uint256 newQuorumNumerator) internal virtual { + require( + newQuorumNumerator <= quorumDenominator(), + "GovernorVotesQuorumFraction: quorumNumerator over quorumDenominator" + ); + + uint256 oldQuorumNumerator = quorumNumerator(); + + // Make sure we keep track of the original numerator in contracts upgraded from a version without checkpoints. + if (oldQuorumNumerator != 0 && _quorumNumeratorHistory._checkpoints.length == 0) { + _quorumNumeratorHistory._checkpoints.push( + Checkpoints.Checkpoint224({ + _key: 0, + _value: SafeCast.toUint224(oldQuorumNumerator) + }) + ); + } + + // Set new quorum for future proposals + _quorumNumeratorHistory.push( + SafeCast.toUint32(clock()), + SafeCast.toUint224(newQuorumNumerator) + ); + + emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); + } +} diff --git a/test/Governance.test.ts b/test/Governance.test.ts index 29a7a87253..2699abb81c 100644 --- a/test/Governance.test.ts +++ b/test/Governance.test.ts @@ -1187,6 +1187,59 @@ describeP1(`Governance - P${IMPLEMENTATION}`, () => { expect(await governor.votingDelay()).to.equal(VOTING_DELAY) }) + it('Should not be able to execute proposals started in a different era -- regression test 09/04/2024', async () => { + // Context: https://github.com/code-423n4/2024-07-reserve-findings/issues/17 + + const era = await stRSRVotes.currentEra() + + // Propose + const proposeTx = await governor + .connect(addr1) + .propose([backingManager.address], [0], [encodedFunctionCall], proposalDescription) + + const proposeReceipt = await proposeTx.wait(1) + const proposalId = proposeReceipt.events![0].args!.proposalId + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Pending) + + // Wipeout -- change era + await whileImpersonating(backingManager.address, async (signer) => { + await stRSRVotes.connect(signer).seizeRSR(await rsr.balanceOf(stRSRVotes.address)) + }) + expect(await stRSRVotes.currentEra()).to.equal(era.add(1)) + + // Stake again + await rsr.connect(addr1).approve(stRSRVotes.address, await rsr.balanceOf(addr1.address)) + await stRSRVotes.connect(addr1).stake(await rsr.balanceOf(addr1.address)) + + // Advance time to start voting + await advanceBlocks(VOTING_DELAY + 1) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Active) + + // vote + await governor.connect(addr1).castVote(proposalId, 1) + await advanceBlocks(1) + + // Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Active) + + // Advance time till voting is complete + await advanceBlocks(VOTING_PERIOD + 1) + + // Finished voting - Check proposal state + expect(await governor.state(proposalId)).to.equal(ProposalState.Succeeded) + + // Cannot queue proposal -- deadend + await expect( + governor + .connect(addr1) + .queue([backingManager.address], [0], [encodedFunctionCall], proposalDescHash) + ).to.be.revertedWith('new era') + }) + it('Should be able to call resetStakes() -- regression test 09/04/2024', async () => { // Context: https://github.com/code-423n4/2024-07-reserve-findings/issues/36 From 56f5e65dd4dee249ec252642e96e63e35cc660eb Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 10:22:13 -0400 Subject: [PATCH 10/14] QA (#1188) --- common/configuration.ts | 1 + contracts/libraries/Fixed.sol | 10 +++++----- contracts/p0/BackingManager.sol | 2 +- contracts/p0/BasketHandler.sol | 6 ++++-- contracts/p0/RToken.sol | 1 + contracts/p0/mixins/Trading.sol | 2 +- contracts/p0/mixins/TradingLib.sol | 2 +- contracts/p1/BackingManager.sol | 2 +- contracts/p1/BasketHandler.sol | 11 ++++++++--- contracts/p1/Furnace.sol | 2 +- contracts/p1/RToken.sol | 1 + contracts/p1/StRSR.sol | 4 ++-- contracts/p1/mixins/TradeLib.sol | 2 +- contracts/p1/mixins/Trading.sol | 2 +- contracts/plugins/assets/Asset.sol | 4 ++-- contracts/plugins/trading/DutchTrade.sol | 10 ++++++++-- contracts/plugins/trading/GnosisTrade.sol | 4 ++-- docs/recollateralization.md | 2 +- test/Main.test.ts | 13 ++++++++++++- test/RToken.test.ts | 7 +++++++ test/ZTradingExtremes.test.ts | 15 ++++++++++++--- .../individual-collateral/collateralTests.ts | 4 ++-- .../curve/collateralTests.ts | 4 ++-- 23 files changed, 77 insertions(+), 34 deletions(-) diff --git a/common/configuration.ts b/common/configuration.ts index b93e8b90d5..d88ab6218a 100644 --- a/common/configuration.ts +++ b/common/configuration.ts @@ -706,6 +706,7 @@ export interface IGovRoles { // System constants export const MAX_TRADE_SLIPPAGE = BigNumber.from(10).pow(18) export const MAX_BACKING_BUFFER = BigNumber.from(10).pow(18) +export const MIN_TARGET_AMT = BigNumber.from(10).pow(12) export const MAX_TARGET_AMT = BigNumber.from(10).pow(21) export const MAX_RATIO = BigNumber.from(10).pow(14) export const MAX_TRADE_VOLUME = BigNumber.from(10).pow(48) diff --git a/contracts/libraries/Fixed.sol b/contracts/libraries/Fixed.sol index 408479ee3f..c075b79deb 100644 --- a/contracts/libraries/Fixed.sol +++ b/contracts/libraries/Fixed.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: BlueOak-1.0.0 // solhint-disable func-name-mixedcase func-visibility // slither-disable-start divide-before-multiply -pragma solidity ^0.8.19; +pragma solidity 0.8.19; /// @title FixedPoint, a fixed-point arithmetic library defining the custom type uint192 /// @author Matt Elder and the Reserve Team @@ -11,7 +11,7 @@ pragma solidity ^0.8.19; "fixed192x18" -- a value represented by 192 bits, that makes 18 digits available to the right of the decimal point. - The range of values that uint192 can represent is about [-1.7e20, 1.7e20]. + The range of values that uint192 can represent is [0, 2^192-1 / 10^18 = 6.2e39]. Unless a function explicitly says otherwise, it will fail on overflow. To be clear, the following should hold: toFix(0) == 0 @@ -44,8 +44,8 @@ uint64 constant FIX_SCALE = 1e18; // FIX_SCALE Squared: uint128 constant FIX_SCALE_SQ = 1e36; -// The largest integer that can be converted to uint192 . -// This is a bit bigger than 3.1e39 +// The largest integer that can be converted to uint192. +// This is a bit bigger than 6.2e39 uint192 constant FIX_MAX_INT = type(uint192).max / FIX_SCALE; uint192 constant FIX_ZERO = 0; // The uint192 representation of zero. @@ -100,7 +100,7 @@ function shiftl_toFix( // conditions for avoiding overflow if (x == 0) return 0; if (shiftLeft <= -96) return (rounding == CEIL ? 1 : 0); // 0 < uint.max / 10**77 < 0.5 - if (40 <= shiftLeft) revert UIntOutOfBounds(); // 10**56 < FIX_MAX < 10**57 + if (40 <= shiftLeft) revert UIntOutOfBounds(); // 10**57 < FIX_MAX < 10**58 shiftLeft += 18; diff --git a/contracts/p0/BackingManager.sol b/contracts/p0/BackingManager.sol index 6bbd2c2a61..00feb14b2d 100644 --- a/contracts/p0/BackingManager.sol +++ b/contracts/p0/BackingManager.sol @@ -97,7 +97,7 @@ contract BackingManagerP0 is TradingP0, IBackingManager { ); require(!main.basketHandler().fullyCollateralized(), "already collateralized"); - // First dissolve any held RToken balance above Distributor-dust + // First dissolve any held RToken balance // gas-optimization: 1 whole RToken must be worth 100 trillion dollars for this to skip $1 uint256 balance = main.rToken().balanceOf(address(this)); if (balance >= MAX_DISTRIBUTION * MAX_DESTINATIONS) main.rToken().dissolve(balance); diff --git a/contracts/p0/BasketHandler.sol b/contracts/p0/BasketHandler.sol index e7c132acd2..a828d2986b 100644 --- a/contracts/p0/BasketHandler.sol +++ b/contracts/p0/BasketHandler.sol @@ -113,6 +113,7 @@ contract BasketHandlerP0 is ComponentP0, IBasketHandler { uint48 public constant MIN_WARMUP_PERIOD = 60; // {s} 1 minute uint48 public constant MAX_WARMUP_PERIOD = 31536000; // {s} 1 year + uint192 public constant MIN_TARGET_AMT = FIX_ONE / 1e6; // {target/BU} min basket weight: 1e-6 uint192 public constant MAX_TARGET_AMT = 1e3 * FIX_ONE; // {target/BU} max basket weight uint256 internal constant MAX_BACKUP_ERC20S = 64; @@ -268,7 +269,8 @@ contract BasketHandlerP0 is ComponentP0, IBasketHandler { /// Set the prime basket in the basket configuration, in terms of erc20s and target amounts /// @param erc20s The collateral for the new prime basket /// @param targetAmts The target amounts (in) {target/BU} for the new prime basket - /// @param disableTargetAmountCheck If true, skips the `requireConstantConfigTargets()` check + /// @param disableTargetAmountCheck For reweightable RTokens, if true + /// skips the `requireConstantConfigTargets()` check /// @custom:governance // checks: // caller is OWNER @@ -313,7 +315,7 @@ contract BasketHandlerP0 is ComponentP0, IBasketHandler { // This is a nice catch to have, but in general it is possible for // an ERC20 in the prime basket to have its asset unregistered. require(reg.toAsset(erc20s[i]).isCollateral(), "erc20 is not collateral"); - require(0 < targetAmts[i], "invalid target amount"); + require(MIN_TARGET_AMT <= targetAmts[i], "invalid target amount"); require(targetAmts[i] <= MAX_TARGET_AMT, "invalid target amount"); config.erc20s.push(erc20s[i]); diff --git a/contracts/p0/RToken.sol b/contracts/p0/RToken.sol index ec3ba5ecd9..640fde3afe 100644 --- a/contracts/p0/RToken.sol +++ b/contracts/p0/RToken.sol @@ -140,6 +140,7 @@ contract RTokenP0 is ComponentP0, ERC20PermitUpgradeable, IRToken { main.poke(); require(amount > 0, "Cannot redeem zero"); + require(recipient != address(0), "cannot redeem to zero address"); require(amount <= balanceOf(_msgSender()), "insufficient balance"); require(main.basketHandler().fullyCollateralized(), "partial redemption; use redeemCustom"); // redemption while IFFY/DISABLED allowed diff --git a/contracts/p0/mixins/Trading.sol b/contracts/p0/mixins/Trading.sol index a8e64d2a7f..1dd27487ef 100644 --- a/contracts/p0/mixins/Trading.sol +++ b/contracts/p0/mixins/Trading.sol @@ -111,7 +111,7 @@ abstract contract TradingP0 is RewardableP0, ITrading { /// @custom:governance function setMaxTradeSlippage(uint192 val) public governance { - require(val < MAX_TRADE_SLIPPAGE, "invalid maxTradeSlippage"); + require(val <= MAX_TRADE_SLIPPAGE, "invalid maxTradeSlippage"); emit MaxTradeSlippageSet(maxTradeSlippage, val); maxTradeSlippage = val; } diff --git a/contracts/p0/mixins/TradingLib.sol b/contracts/p0/mixins/TradingLib.sol index ab4232dbd8..4d18250a3d 100644 --- a/contracts/p0/mixins/TradingLib.sol +++ b/contracts/p0/mixins/TradingLib.sol @@ -60,7 +60,7 @@ library TradingLibP0 { ); // Cap sell amount using the high price - // Under price decay trade.prices.sellHigh can become up to 3x the savedHighPrice before + // Under price decay trade.prices.sellHigh can become up to 2x the savedHighPrice before // becoming FIX_MAX after the full price timeout uint192 s = trade.sellAmount; if (trade.prices.sellHigh != FIX_MAX) { diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol index 4724cb9c36..a5837bc6b9 100644 --- a/contracts/p1/BackingManager.sol +++ b/contracts/p1/BackingManager.sol @@ -125,7 +125,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager { require(basketsHeld.bottom < rToken.basketsNeeded(), "already collateralized"); // require(!basketHandler.fullyCollateralized()) - // First dissolve any held RToken balance (above Distributor-dust) + // First dissolve any held RToken balance // gas-optimization: 1 whole RToken must be worth 100 trillion dollars for this to skip $1 uint256 balance = rToken.balanceOf(address(this)); if (balance >= MAX_DISTRIBUTION * MAX_DESTINATIONS) rToken.dissolve(balance); diff --git a/contracts/p1/BasketHandler.sol b/contracts/p1/BasketHandler.sol index 4a07a81024..b67bccf2ea 100644 --- a/contracts/p1/BasketHandler.sol +++ b/contracts/p1/BasketHandler.sol @@ -28,7 +28,8 @@ contract BasketHandlerP1 is ComponentP1, IBasketHandler { using EnumerableSet for EnumerableSet.Bytes32Set; using FixLib for uint192; - uint192 public constant MAX_TARGET_AMT = 1e3 * FIX_ONE; // {target/BU} max basket weight + uint192 public constant MIN_TARGET_AMT = FIX_ONE / 1e6; // {target/BU} min basket weight: 1e-6 + uint192 public constant MAX_TARGET_AMT = 1e3 * FIX_ONE; // {target/BU} max basket weight: 1e3 uint48 public constant MIN_WARMUP_PERIOD = 60; // {s} 1 minute uint48 public constant MAX_WARMUP_PERIOD = 60 * 60 * 24 * 365; // {s} 1 year uint256 internal constant MAX_BACKUP_ERC20S = 64; @@ -211,7 +212,8 @@ contract BasketHandlerP1 is ComponentP1, IBasketHandler { /// Set the prime basket in the basket configuration, in terms of erc20s and target amounts /// @param erc20s The collateral for the new prime basket /// @param targetAmts The target amounts (in) {target/BU} for the new prime basket - /// @param disableTargetAmountCheck If true, skips the `requireConstantConfigTargets()` check + /// @param disableTargetAmountCheck For reweightable RTokens, if true + /// skips the `requireConstantConfigTargets()` check /// @custom:governance // checks: // caller is OWNER @@ -261,7 +263,10 @@ contract BasketHandlerP1 is ComponentP1, IBasketHandler { // This is a nice catch to have, but in general it is possible for // an ERC20 in the prime basket to have its asset unregistered. require(assetRegistry.toAsset(erc20s[i]).isCollateral(), "erc20 is not collateral"); - require(0 < targetAmts[i] && targetAmts[i] <= MAX_TARGET_AMT, "invalid target amount"); + require( + MIN_TARGET_AMT <= targetAmts[i] && targetAmts[i] <= MAX_TARGET_AMT, + "invalid target amount" + ); config.erc20s.push(erc20s[i]); config.targetAmts[erc20s[i]] = targetAmts[i]; diff --git a/contracts/p1/Furnace.sol b/contracts/p1/Furnace.sol index ce65164002..1ea0a43581 100644 --- a/contracts/p1/Furnace.sol +++ b/contracts/p1/Furnace.sol @@ -58,7 +58,7 @@ contract FurnaceP1 is ComponentP1, IFurnace { // payoutAmount = RToken.balanceOf(this) * (1 - (1-ratio)**N) from [furnace-payout-formula] // effects: // lastPayout' = lastPayout + numPeriods (end of last pay period) - // lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay leriod) + // lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay period) // actions: // rToken.melt(payoutAmount), paying payoutAmount to RToken holders diff --git a/contracts/p1/RToken.sol b/contracts/p1/RToken.sol index 63308e58ea..6e2dfa80a6 100644 --- a/contracts/p1/RToken.sol +++ b/contracts/p1/RToken.sol @@ -189,6 +189,7 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { address caller = _msgSender(); require(amount != 0, "Cannot redeem zero"); + require(recipient != address(0), "cannot redeem to zero address"); require(amount <= balanceOf(caller), "insufficient balance"); require(basketHandler.fullyCollateralized(), "partial redemption; use redeemCustom"); // redemption while IFFY/DISABLED allowed diff --git a/contracts/p1/StRSR.sol b/contracts/p1/StRSR.sol index 6e7b1e6f55..aa6342d87f 100644 --- a/contracts/p1/StRSR.sol +++ b/contracts/p1/StRSR.sol @@ -326,10 +326,10 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab uint256 newDraftRSR = (newTotalDrafts * FIX_ONE_256 + (draftRate - 1)) / draftRate; uint256 rsrAmount = draftRSR - newDraftRSR; - if (rsrAmount == 0) return; - // ==== Transfer RSR from the draft pool totalDrafts = newTotalDrafts; + if (rsrAmount == 0) return; + draftRSR = newDraftRSR; // == Interactions == diff --git a/contracts/p1/mixins/TradeLib.sol b/contracts/p1/mixins/TradeLib.sol index e989ea81e9..83e565e63d 100644 --- a/contracts/p1/mixins/TradeLib.sol +++ b/contracts/p1/mixins/TradeLib.sol @@ -59,7 +59,7 @@ library TradeLib { ); // Cap sell amount using the high price - // Under price decay trade.prices.sellHigh can become up to 3x the savedHighPrice before + // Under price decay trade.prices.sellHigh can become up to 2x the savedHighPrice before // becoming FIX_MAX after the full price timeout uint192 s = trade.sellAmount; if (trade.prices.sellHigh != FIX_MAX) { diff --git a/contracts/p1/mixins/Trading.sol b/contracts/p1/mixins/Trading.sol index 0798cb8cdc..49c06a5810 100644 --- a/contracts/p1/mixins/Trading.sol +++ b/contracts/p1/mixins/Trading.sol @@ -157,7 +157,7 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl /// @custom:governance function setMaxTradeSlippage(uint192 val) public governance { - require(val < MAX_TRADE_SLIPPAGE, "invalid maxTradeSlippage"); + require(val <= MAX_TRADE_SLIPPAGE, "invalid maxTradeSlippage"); emit MaxTradeSlippageSet(maxTradeSlippage, val); maxTradeSlippage = val; } diff --git a/contracts/plugins/assets/Asset.sol b/contracts/plugins/assets/Asset.sol index 5c29ac48b1..b13eed30b2 100644 --- a/contracts/plugins/assets/Asset.sol +++ b/contracts/plugins/assets/Asset.sol @@ -11,7 +11,7 @@ contract Asset is IAsset, VersionedAsset { using FixLib for uint192; using OracleLib for AggregatorV3Interface; - uint192 public constant MAX_HIGH_PRICE_BUFFER = 2 * FIX_ONE; // {UoA/tok} 200% + uint192 public constant MAX_HIGH_PRICE_BUFFER = FIX_ONE; // {UoA/tok} 100% AggregatorV3Interface public immutable chainlinkFeed; // {UoA/tok} @@ -147,7 +147,7 @@ contract Asset is IAsset, VersionedAsset { } else { // decayDelay <= delta <= decayDelay + priceTimeout - // Decay _high upwards to 3x savedHighPrice + // Decay _high upwards to 2x savedHighPrice // {UoA/tok} = {UoA/tok} * {1} _high = savedHighPrice.safeMul( FIX_ONE + MAX_HIGH_PRICE_BUFFER.muluDivu(delta - decayDelay, priceTimeout), diff --git a/contracts/plugins/trading/DutchTrade.sol b/contracts/plugins/trading/DutchTrade.sol index 65004ce613..9c42e23383 100644 --- a/contracts/plugins/trading/DutchTrade.sol +++ b/contracts/plugins/trading/DutchTrade.sol @@ -165,8 +165,14 @@ contract DutchTrade is ITrade, Versioned { assert(address(sell_) != address(0) && address(buy_) != address(0) && auctionLength >= 60); // Only start dutch auctions under well-defined prices - require(prices.sellLow != 0 && prices.sellHigh < FIX_MAX / 1000, "bad sell pricing"); - require(prices.buyLow != 0 && prices.buyHigh < FIX_MAX / 1000, "bad buy pricing"); + require( + prices.sellLow != 0 && prices.sellHigh != 0 && prices.sellHigh < FIX_MAX / 1000, + "bad sell pricing" + ); + require( + prices.buyLow != 0 && prices.buyHigh != 0 && prices.buyHigh < FIX_MAX / 1000, + "bad buy pricing" + ); broker = IBroker(msg.sender); origin = origin_; diff --git a/contracts/plugins/trading/GnosisTrade.sol b/contracts/plugins/trading/GnosisTrade.sol index c9385a1dc2..2cc4f7501c 100644 --- a/contracts/plugins/trading/GnosisTrade.sol +++ b/contracts/plugins/trading/GnosisTrade.sol @@ -47,7 +47,7 @@ contract GnosisTrade is ITrade, Versioned { // == Economic parameters // This trade is on behalf of origin. Only origin may call settle(), and the `buy` tokens - // from this trade's acution will all eventually go to origin. + // from this trade's auction will all eventually go to origin. address public origin; IERC20Metadata public sell; // address of token this trade is selling IERC20Metadata public buy; // address of token this trade is buying @@ -56,7 +56,7 @@ contract GnosisTrade is ITrade, Versioned { uint48 public endTime; // timestamp after which this trade's auction can be settled uint192 public worstCasePrice; // D27{qBuyTok/qSellTok}, the worst price we expect to get // We expect Gnosis Auction either to meet or beat worstCasePrice, or to return the `sell` - // tokens. If we actually *get* a worse clearing that worstCasePrice, we consider it an error in + // tokens. If we actually *get* a worse clearing than worstCasePrice, we consider it an error in // our trading scheme and call broker.reportViolation() // This modifier both enforces the state-machine pattern and guards against reentrancy. diff --git a/docs/recollateralization.md b/docs/recollateralization.md index 32ddb9c97c..fc91ed2e3c 100644 --- a/docs/recollateralization.md +++ b/docs/recollateralization.md @@ -60,7 +60,7 @@ This allows the protocol to deterministically select the next trade based on the (Caveat: if the protocol gets an unreasonably good trade in excess of what was indicated by an asset's price range, this can happen) 5. Large trades first, as determined by comparison in the `{UoA}` -If there does not exist a trade that meets these constraints, then the protocol "takes a haircut", which is a colloquial way of saying it reduces `RToken.basketsNeeded()` to its current BU holdings. This causes a loss for RToken holders (undesirable) but causes the protocol to become collateralized again, allowing it to re-enter into a period of normal operation. +If there does not exist a trade that meets these constraints, the protocol considers the RSR balance in StRSR before moving to "take a haircut", which is a colloquial way of saying it reduces `RToken.basketsNeeded()` to its current BU holdings to become by-definition collateralized. This causes a loss for RToken holders (undesirable) but causes the protocol to regain normal function. #### Trade Sizing diff --git a/test/Main.test.ts b/test/Main.test.ts index ca1d7dfad7..3ce2b12c2b 100644 --- a/test/Main.test.ts +++ b/test/Main.test.ts @@ -8,6 +8,7 @@ import { MAX_TRADING_DELAY, MAX_TRADE_SLIPPAGE, MAX_BACKING_BUFFER, + MIN_TARGET_AMT, MAX_TARGET_AMT, MAX_MIN_TRADE_VOLUME, MIN_WARMUP_PERIOD, @@ -1039,7 +1040,7 @@ describe(`MainP${IMPLEMENTATION} contract`, () => { // Cannot update with value > max await expect( - backingManager.connect(owner).setMaxTradeSlippage(MAX_TRADE_SLIPPAGE) + backingManager.connect(owner).setMaxTradeSlippage(MAX_TRADE_SLIPPAGE.add(1)) ).to.be.revertedWith('invalid maxTradeSlippage') }) @@ -2157,6 +2158,16 @@ describe(`MainP${IMPLEMENTATION} contract`, () => { ).to.be.revertedWith('invalid collateral') }) + it('Should not allow to bypass MIN_TARGET_AMT', async () => { + // not possible on non-fresh basketHandler + await expect( + indexBH.connect(owner).setPrimeBasket([token0.address], [MIN_TARGET_AMT.sub(1)]) + ).to.be.revertedWith('invalid target amount') + await expect( + indexBH.connect(owner).forceSetPrimeBasket([token0.address], [MIN_TARGET_AMT.sub(1)]) + ).to.be.revertedWith('invalid target amount') + }) + it('Should not allow to bypass MAX_TARGET_AMT', async () => { // not possible on non-fresh basketHandler await expect( diff --git a/test/RToken.test.ts b/test/RToken.test.ts index 6d5f2a54db..e895433213 100644 --- a/test/RToken.test.ts +++ b/test/RToken.test.ts @@ -319,6 +319,13 @@ describe(`RTokenP${IMPLEMENTATION} contract`, () => { expect(endPrice[0]).to.eq(0) expect(endPrice[1]).to.eq(0) }) + + it('Should not allow redemption to zero address', async function () { + // Redeem rTokens to zero address + await expect(rToken.connect(addr1).redeemTo(ZERO_ADDRESS, fp('1'))).to.be.revertedWith( + 'cannot redeem to zero address' + ) + }) }) describe('Issuance', function () { diff --git a/test/ZTradingExtremes.test.ts b/test/ZTradingExtremes.test.ts index 8ff6b8cc30..e566f5d062 100644 --- a/test/ZTradingExtremes.test.ts +++ b/test/ZTradingExtremes.test.ts @@ -5,6 +5,7 @@ import { BigNumber, ContractFactory } from 'ethers' import { ethers } from 'hardhat' import { IConfig, + MIN_TARGET_AMT, MAX_ORACLE_TIMEOUT, MAX_THROTTLE_AMT_RATE, MAX_BASKET_SIZE, @@ -415,7 +416,11 @@ describeExtreme(`Trading Extreme Values (${SLOW ? 'slow mode' : 'fast mode'})`, } primeBasket.push(token) - targetAmts.push(divCeil(primeWeight, bn(basketSize))) // might sum to slightly over, is ok + + let targetAmt = divCeil(primeWeight, bn(basketSize)) + if (targetAmt.lt(MIN_TARGET_AMT)) targetAmt = MIN_TARGET_AMT + targetAmts.push(targetAmt) // might sum to slightly over, is ok + await token.connect(owner).mint(addr1.address, MAX_UINT256) await token.connect(addr1).approve(rToken.address, MAX_UINT256) } @@ -788,7 +793,11 @@ describeExtreme(`Trading Extreme Values (${SLOW ? 'slow mode' : 'fast mode'})`, } primeBasket.push(token) - targetAmts.push(primeWeight.div(basketSize).add(1)) + + let targetAmt = divCeil(primeWeight, bn(basketSize)) + if (targetAmt.lt(MIN_TARGET_AMT)) targetAmt = MIN_TARGET_AMT + targetAmts.push(targetAmt) // might sum to slightly over, is ok + await token.connect(owner).mint(addr1.address, MAX_UINT256) await token.connect(addr1).approve(rToken.address, MAX_UINT256) } @@ -938,7 +947,7 @@ describeExtreme(`Trading Extreme Values (${SLOW ? 'slow mode' : 'fast mode'})`, const erc20 = await makeToken(`Token ${i}`, targetUnit, targetPerRefs) primeERC20s.push(erc20.address) let targetAmt = basketTargetAmt.div(targetUnits) - if (targetAmt.eq(bn(0))) targetAmt = bn(1) + if (targetAmt.lt(MIN_TARGET_AMT)) targetAmt = MIN_TARGET_AMT targetAmts.push(targetAmt) } diff --git a/test/plugins/individual-collateral/collateralTests.ts b/test/plugins/individual-collateral/collateralTests.ts index 8f62c5ffa9..66119a1fe9 100644 --- a/test/plugins/individual-collateral/collateralTests.ts +++ b/test/plugins/individual-collateral/collateralTests.ts @@ -428,8 +428,8 @@ export default function fn( const priceTimeout = await collateral.priceTimeout() await advanceTime(priceTimeout / 2) p = await collateral.price() - expect(p[0]).to.be.closeTo(savedLow.div(2), p[0].div(2).div(10000)) // 1 part in 10 thousand - expect(p[1]).to.be.closeTo(savedHigh.mul(2), p[1].mul(2).div(10000)) // 1 part in 10 thousand + expect(p[0]).to.be.closeTo(savedLow.div(2), savedLow.div(2).div(10000)) // 1 part in 10 thousand + expect(p[1]).to.be.closeTo(savedHigh.mul(3).div(2), savedHigh.mul(3).div(2).div(10000)) // 1 part in 10k // Should be unpriced after full priceTimeout await advanceTime(priceTimeout / 2) diff --git a/test/plugins/individual-collateral/curve/collateralTests.ts b/test/plugins/individual-collateral/curve/collateralTests.ts index 48c28790d6..0ae2e202be 100644 --- a/test/plugins/individual-collateral/curve/collateralTests.ts +++ b/test/plugins/individual-collateral/curve/collateralTests.ts @@ -500,8 +500,8 @@ export default function fn( const priceTimeout = await ctx.collateral.priceTimeout() await advanceTime(priceTimeout / 2) p = await ctx.collateral.price() - expect(p[0]).to.be.closeTo(savedLow.div(2), p[0].div(2).div(10000)) // 1 part in 10 thousand - expect(p[1]).to.be.closeTo(savedHigh.mul(2), p[1].mul(2).div(10000)) // 1 part in 10 thousand + expect(p[0]).to.be.closeTo(savedLow.div(2), savedLow.div(2).div(10000)) // 1 part in 10 thousand + expect(p[1]).to.be.closeTo(savedHigh.mul(3).div(2), savedHigh.mul(3).div(2).div(10000)) // 1 part in 10k // Should be 0 after full priceTimeout await advanceTime(priceTimeout / 2) From 95231c0be3bc454df0809c46759345f7652e87b2 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 12:15:09 -0400 Subject: [PATCH 11/14] update stakeRate/draftRate only when well-defined (#1198) --- contracts/p1/StRSR.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/p1/StRSR.sol b/contracts/p1/StRSR.sol index aa6342d87f..d251d1c0ac 100644 --- a/contracts/p1/StRSR.sol +++ b/contracts/p1/StRSR.sol @@ -443,7 +443,7 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab seizedRSR = stakeRSRToTake; // update stakeRate, possibly beginning a new stake era - if (stakeRSR != 0) { + if (stakeRSR != 0 && totalStakes != 0) { // Downcast is safe: totalStakes is 1e38 at most so expression maximum value is 1e56 stakeRate = uint192((FIX_ONE_256 * totalStakes + (stakeRSR - 1)) / stakeRSR); } @@ -458,11 +458,10 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab seizedRSR += draftRSRToTake; // update draftRate, possibly beginning a new draft era - if (draftRSR != 0) { + if (draftRSR != 0 && totalDrafts != 0) { // Downcast is safe: totalDrafts is 1e38 at most so expression maximum value is 1e56 draftRate = uint192((FIX_ONE_256 * totalDrafts + (draftRSR - 1)) / draftRSR); } - if (draftRSR == 0 || draftRate > MAX_DRAFT_RATE) { seizedRSR += draftRSR; beginDraftEra(); From 36ab2decc7cac66c51ac620351da9c0983b495df Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 14:58:16 -0400 Subject: [PATCH 12/14] allow resetStakes when either rate is unsafe (#1199) --- contracts/p0/StRSR.sol | 11 +++++++++-- contracts/p1/StRSR.sol | 16 ++++++++++++---- test/ZZStRSR.test.ts | 8 ++++---- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/contracts/p0/StRSR.sol b/contracts/p0/StRSR.sol index 4a3c0a1da9..bd89e0890d 100644 --- a/contracts/p0/StRSR.sol +++ b/contracts/p0/StRSR.sol @@ -102,6 +102,8 @@ contract StRSRP0 is IStRSR, ComponentP0, EIP712Upgradeable { // stake rate under/over which governance can reset all stakes uint192 private constant MAX_SAFE_STAKE_RATE = 1e6 * FIX_ONE; // 1e6 uint192 private constant MIN_SAFE_STAKE_RATE = uint192(1e12); // 1e-6 + uint192 private constant MAX_SAFE_DRAFT_RATE = 1e6 * FIX_ONE; // 1e6 + uint192 private constant MIN_SAFE_DRAFT_RATE = uint192(1e12); // 1e-6 // Withdrawal Leak uint192 private leaked; // {1} stake fraction that has withdrawn without a refresh @@ -386,10 +388,15 @@ contract StRSRP0 is IStRSR, ComponentP0, EIP712Upgradeable { /// @custom:governance /// Reset all stakes and advance era function resetStakes() external governance { + uint256 rsrDrafts = rsrBeingWithdrawn(); + uint192 draftRate = rsrDrafts > 0 ? divuu(stakeBeingWithdrawn(), rsrDrafts) : FIX_ONE; uint192 stakeRate = divuu(totalStaked, rsrBacking); require( - stakeRate <= MIN_SAFE_STAKE_RATE || stakeRate >= MAX_SAFE_STAKE_RATE, - "rate still safe" + draftRate <= MIN_SAFE_DRAFT_RATE || + draftRate >= MAX_SAFE_DRAFT_RATE || + stakeRate <= MIN_SAFE_STAKE_RATE || + stakeRate >= MAX_SAFE_STAKE_RATE, + "rates still safe" ); bankruptStakers(); diff --git a/contracts/p1/StRSR.sol b/contracts/p1/StRSR.sol index d251d1c0ac..190aeb6258 100644 --- a/contracts/p1/StRSR.sol +++ b/contracts/p1/StRSR.sol @@ -163,6 +163,8 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab // stake rate under/over which governance can reset all stakes uint192 private constant MAX_SAFE_STAKE_RATE = 1e6 * FIX_ONE; // 1e6 D18{qStRSR/qRSR} uint192 private constant MIN_SAFE_STAKE_RATE = uint192(1e12); // 1e-6 D18{qStRSR/qRSR} + uint192 private constant MAX_SAFE_DRAFT_RATE = 1e6 * FIX_ONE; // 1e6 D18{qStRSR/qRSR} + uint192 private constant MIN_SAFE_DRAFT_RATE = uint192(1e12); // 1e-6 D18{qStRSR/qRSR} // ====================== @@ -478,19 +480,25 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab /// @custom:governance /// Reset all stakes and advance era - /// @notice This function is only callable when the stake rate is unsafe. - /// The stake rate is unsafe when it is either too high or too low. + /// @notice This function is only callable when the stake or draft rates are unsafe. + /// The stake/draft rate is unsafe when it is either too high or too low. /// There is the possibility of the rate reaching the borderline of being unsafe, /// where users won't stake in fear that a reset might be executed. /// A user may also grief this situation by staking enough RSR to vote against any reset. /// This standoff will continue until enough RSR is staked and a reset is executed. /// There is currently no good and easy way to mitigate the possibility of this situation, /// and the risk of it occurring is low enough that it is not worth the effort to mitigate. + /// @notice Governance must monitor the draftRate! After multiple seizures it is possible + /// for it to drift near the unsafe bounds. Even if stakes are still safe at this point, + /// resetStakes() should be called by governance anyway. function resetStakes() external { _requireGovernanceOnly(); require( - stakeRate <= MIN_SAFE_STAKE_RATE || stakeRate >= MAX_SAFE_STAKE_RATE, - "rate still safe" + draftRate <= MIN_SAFE_DRAFT_RATE || + draftRate >= MAX_SAFE_DRAFT_RATE || + stakeRate <= MIN_SAFE_STAKE_RATE || + stakeRate >= MAX_SAFE_STAKE_RATE, + "rates still safe" ); beginEra(); diff --git a/test/ZZStRSR.test.ts b/test/ZZStRSR.test.ts index 06fba12cc2..ea7e7356c0 100644 --- a/test/ZZStRSR.test.ts +++ b/test/ZZStRSR.test.ts @@ -2226,7 +2226,7 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => { expect(await stRSR.balanceOf(addr1.address)).to.equal(stakeAmt) // Cannot reset stakes with this rate - await expect(stRSR.connect(owner).resetStakes()).to.be.revertedWith('rate still safe') + await expect(stRSR.connect(owner).resetStakes()).to.be.revertedWith('rates still safe') // Seize small portion of RSR to increase stake rate - still safe await whileImpersonating(backingManager.address, async (signer) => { @@ -2240,7 +2240,7 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => { expect(await stRSR.balanceOf(addr1.address)).to.equal(stakeAmt) // Attempt to reset stakes, still not possible - await expect(stRSR.connect(owner).resetStakes()).to.be.revertedWith('rate still safe') + await expect(stRSR.connect(owner).resetStakes()).to.be.revertedWith('rates still safe') // New Seizure - rate will be unsafe const rsrRemaining = stakeAmt.sub(seizeAmt) @@ -2278,7 +2278,7 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => { expect(await stRSR.balanceOf(addr1.address)).to.equal(stakeAmt) // Cannot reset stakes with this rate - await expect(stRSR.connect(owner).resetStakes()).to.be.revertedWith('rate still safe') + await expect(stRSR.connect(owner).resetStakes()).to.be.revertedWith('rates still safe') // Add RSR to decrease stake rate - still safe await rsr.connect(owner).transfer(stRSR.address, addAmt1) @@ -2300,7 +2300,7 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => { expect(await stRSR.balanceOf(addr1.address)).to.equal(stakeAmt) // Attempt to reset stakes, still not possible - await expect(stRSR.connect(owner).resetStakes()).to.be.revertedWith('rate still safe') + await expect(stRSR.connect(owner).resetStakes()).to.be.revertedWith('rates still safe') // Add a large amount of funds - rate will be unsafe await rsr.connect(owner).mint(owner.address, addAmt2) From cee7bc2ee0f86cdecd59dd3bf133fa2fc7ebe3f1 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 14:58:58 -0400 Subject: [PATCH 13/14] unblock rebalancing after an auction ends early (#1195) --- contracts/p0/BackingManager.sol | 3 +++ contracts/p1/BackingManager.sol | 3 +++ test/Recollateralization.test.ts | 25 +++++++++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/contracts/p0/BackingManager.sol b/contracts/p0/BackingManager.sol index 00feb14b2d..de11a8a827 100644 --- a/contracts/p0/BackingManager.sol +++ b/contracts/p0/BackingManager.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.19; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts/utils/math/Math.sol"; import "./mixins/TradingLib.sol"; import "./mixins/Trading.sol"; import "../interfaces/IAsset.sol"; @@ -65,6 +66,8 @@ contract BackingManagerP0 is TradingP0, IBackingManager { trade = super.settleTrade(sell); delete tokensOut[trade.sell()]; + tradeEnd[trade.KIND()] = uint48(Math.min(tradeEnd[trade.KIND()], block.timestamp)); + // if the settler is the trade contract itself, try chaining with another rebalance() if (_msgSender() == address(trade)) { // solhint-disable-next-line no-empty-blocks diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol index a5837bc6b9..2129eab875 100644 --- a/contracts/p1/BackingManager.sol +++ b/contracts/p1/BackingManager.sol @@ -86,6 +86,9 @@ contract BackingManagerP1 is TradingP1, IBackingManager { delete tokensOut[sell]; trade = super.settleTrade(sell); // nonReentrant + TradeKind kind = trade.KIND(); + if (tradeEnd[kind] > block.timestamp) tradeEnd[kind] = uint48(block.timestamp); + // if the settler is the trade contract itself, try chaining with another rebalance() if (_msgSender() == address(trade)) { // solhint-disable-next-line no-empty-blocks diff --git a/test/Recollateralization.test.ts b/test/Recollateralization.test.ts index d2d32b059f..d19467fbb8 100644 --- a/test/Recollateralization.test.ts +++ b/test/Recollateralization.test.ts @@ -3412,11 +3412,36 @@ describe(`Recollateralization - P${IMPLEMENTATION}`, () => { expect(await trade2.canSettle()).to.equal(false) // Bid + settle RSR auction + await expect(await router.connect(addr1).bid(trade2.address, addr1.address)).to.emit( + backingManager, + 'TradeSettled' + ) + }) + + it('and be able to continue rebalance immediately -- regression test 09/05/2024', async () => { + // Context: https://github.com/code-423n4/2024-07-reserve-findings/issues/6 + + await token1.connect(addr1).approve(trade2.address, initialBal) + + // Advance to midpoint of auction + await advanceToTimestamp((await getLatestBlockTimestamp()) + 900) + expect(await trade2.status()).to.equal(1) // TradeStatus.OPEN + expect(await trade2.canSettle()).to.equal(false) + // Bid + settle RSR auction await expect(await router.connect(addr1).bid(trade2.address, addr1.address)).to.emit( backingManager, 'TradeSettled' ) + expect(await broker.dutchTradeDisabled(token1.address)).to.equal(false) + + // Should be able to immediately continue + const amt = await token1.balanceOf(backingManager.address) + await token1.burn(backingManager.address, amt) + + // Should not revert, if we were to continue + await backingManager.callStatic.rebalance(TradeKind.DUTCH_AUCTION) + await token1.mint(backingManager.address, amt) }) it('via fallback to Batch Auction', async () => { From e29f94f29cdfb17ecf3a68bb5fffa9a44df793e7 Mon Sep 17 00:00:00 2001 From: Taylor Brent Date: Tue, 10 Sep 2024 15:15:35 -0400 Subject: [PATCH 14/14] add test for non-governance forceSettleTrade --- test/Broker.test.ts | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/Broker.test.ts b/test/Broker.test.ts index 0f1ee660da..6eafdd5cda 100644 --- a/test/Broker.test.ts +++ b/test/Broker.test.ts @@ -551,6 +551,31 @@ describe(`BrokerP${IMPLEMENTATION} contract #fast`, () => { expect(await tradeWithReentrantGnosis.canSettle()).to.equal(false) }) + it('Should not allow to force settle a trade unless governance', async () => { + // Initialize trade - simulate from backingManager + const tradeRequest: ITradeRequest = { + sell: collateral0.address, + buy: collateral1.address, + sellAmount: amount, + minBuyAmount: bn('0'), + } + + // Fund trade and initialize + await token0.connect(owner).mint(trade.address, amount) + await expect( + trade.init( + broker.address, + backingManager.address, + config.batchAuctionLength, + tradeRequest + ) + ).to.not.be.reverted + + await expect( + backingManager.connect(addr1).forceSettleTrade(trade.address) + ).to.be.revertedWith('governance only') + }) + it('Should not allow deployment with zero address gnosis', async () => { const GnosisTradeFactory: ContractFactory = await ethers.getContractFactory('GnosisTrade') await expect(GnosisTradeFactory.deploy(ZERO_ADDRESS)).to.be.revertedWith(