From 03b553f926212e6fb5cf7308c7f8b58f08237222 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 15 Mar 2022 10:23:40 -0400 Subject: [PATCH 1/3] fix: [L05] Liquidity provisioning should revert if token already enabled --- contracts/HubPool.sol | 3 +++ test/HubPool.Admin.ts | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c10d7fb4a..623327893 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -411,6 +411,9 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @param l1Token Token to provide liquidity for. */ function enableL1TokenForLiquidityProvision(address l1Token) public override onlyOwner nonReentrant { + // The reason we revert if this L1 token is already enabled is so that the LpFeeUpdate timestamp is not + // reset unnecessarily, as this would cause any LP fees that have accrued since the last timestamp to be lost. + require(!pooledTokens[l1Token].isEnabled, "L1 token already enabled"); if (pooledTokens[l1Token].lpToken == address(0)) pooledTokens[l1Token].lpToken = lpTokenFactory.createLpToken(l1Token); diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index a718e591e..6a25807c4 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -23,6 +23,10 @@ describe("HubPool Admin functions", function () { expect((await hubPool.callStatic.pooledTokens(weth.address)).lpToken).to.equal(zeroAddress); await hubPool.enableL1TokenForLiquidityProvision(weth.address); + await expect(hubPool.enableL1TokenForLiquidityProvision(weth.address)).to.be.revertedWith( + "L1 token already enabled" + ); + const pooledTokenStruct = await hubPool.callStatic.pooledTokens(weth.address); expect(pooledTokenStruct.lpToken).to.not.equal(zeroAddress); expect(pooledTokenStruct.isEnabled).to.equal(true); @@ -36,8 +40,16 @@ describe("HubPool Admin functions", function () { await expect(hubPool.connect(other).enableL1TokenForLiquidityProvision(weth.address)).to.be.reverted; }); it("Can disable L1 Tokens for liquidity provision", async function () { + await hubPool.enableL1TokenForLiquidityProvision(weth.address); + const pooledTokenStruct = await hubPool.callStatic.pooledTokens(weth.address); + const lpToken = pooledTokenStruct.lpToken; + await hubPool.disableL1TokenForLiquidityProvision(weth.address); expect((await hubPool.callStatic.pooledTokens(weth.address)).isEnabled).to.equal(false); + + // Can re-enable the L1 token now without creating a new LP token. + await hubPool.enableL1TokenForLiquidityProvision(weth.address); + expect((await hubPool.callStatic.pooledTokens(weth.address)).lpToken).to.equal(lpToken); }); it("Only owner can disable L1 Tokens for liquidity provision", async function () { await expect(hubPool.connect(other).disableL1TokenForLiquidityProvision(weth.address)).to.be.reverted; From 0b040cfaf045b8fd3e6e153daf6466eeee56b251 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 15 Mar 2022 12:51:12 -0400 Subject: [PATCH 2/3] only reset lastLpFeeUpdate once --- contracts/HubPool.sol | 11 ++++++----- test/HubPool.Admin.ts | 10 ++++------ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 623327893..7befb9796 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -411,14 +411,15 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { * @param l1Token Token to provide liquidity for. */ function enableL1TokenForLiquidityProvision(address l1Token) public override onlyOwner nonReentrant { - // The reason we revert if this L1 token is already enabled is so that the LpFeeUpdate timestamp is not - // reset unnecessarily, as this would cause any LP fees that have accrued since the last timestamp to be lost. - require(!pooledTokens[l1Token].isEnabled, "L1 token already enabled"); - if (pooledTokens[l1Token].lpToken == address(0)) + // If token is being enabled for the first time, create a new LP token and set the timestamp once. We don't + // want to ever reset this timestamp otherwise fees that have accrued will be lost since the last update. This + // could happen for example if an L1 token is enabled, disabled, and then enabled again. + if (pooledTokens[l1Token].lpToken == address(0)) { pooledTokens[l1Token].lpToken = lpTokenFactory.createLpToken(l1Token); + pooledTokens[l1Token].lastLpFeeUpdate = uint32(getCurrentTime()); + } pooledTokens[l1Token].isEnabled = true; - pooledTokens[l1Token].lastLpFeeUpdate = uint32(getCurrentTime()); emit L1TokenEnabledForLiquidityProvision(l1Token, pooledTokens[l1Token].lpToken); } diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index 6a25807c4..2e8ca19ef 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -13,7 +13,7 @@ let hubPool: Contract, identifierWhitelist: Contract; let owner: SignerWithAddress, other: SignerWithAddress; -describe("HubPool Admin functions", function () { +describe.only("HubPool Admin functions", function () { beforeEach(async function () { [owner, other] = await ethers.getSigners(); ({ weth, hubPool, usdc, mockAdapter, mockSpoke, identifierWhitelist } = await hubPoolFixture()); @@ -23,10 +23,6 @@ describe("HubPool Admin functions", function () { expect((await hubPool.callStatic.pooledTokens(weth.address)).lpToken).to.equal(zeroAddress); await hubPool.enableL1TokenForLiquidityProvision(weth.address); - await expect(hubPool.enableL1TokenForLiquidityProvision(weth.address)).to.be.revertedWith( - "L1 token already enabled" - ); - const pooledTokenStruct = await hubPool.callStatic.pooledTokens(weth.address); expect(pooledTokenStruct.lpToken).to.not.equal(zeroAddress); expect(pooledTokenStruct.isEnabled).to.equal(true); @@ -43,13 +39,15 @@ describe("HubPool Admin functions", function () { await hubPool.enableL1TokenForLiquidityProvision(weth.address); const pooledTokenStruct = await hubPool.callStatic.pooledTokens(weth.address); const lpToken = pooledTokenStruct.lpToken; + const lastLpFeeUpdate = pooledTokenStruct.lastLpFeeUpdate; await hubPool.disableL1TokenForLiquidityProvision(weth.address); expect((await hubPool.callStatic.pooledTokens(weth.address)).isEnabled).to.equal(false); - // Can re-enable the L1 token now without creating a new LP token. + // Can re-enable the L1 token now without creating a new LP token or resetting timestamp. await hubPool.enableL1TokenForLiquidityProvision(weth.address); expect((await hubPool.callStatic.pooledTokens(weth.address)).lpToken).to.equal(lpToken); + expect((await hubPool.callStatic.pooledTokens(weth.address)).lastLpFeeUpdate).to.equal(lastLpFeeUpdate); }); it("Only owner can disable L1 Tokens for liquidity provision", async function () { await expect(hubPool.connect(other).disableL1TokenForLiquidityProvision(weth.address)).to.be.reverted; From b1a097748a82c3276619a06fa36358b574f843e1 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 15 Mar 2022 12:52:13 -0400 Subject: [PATCH 3/3] Update HubPool.Admin.ts --- test/HubPool.Admin.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index 2e8ca19ef..09e431d1d 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -13,7 +13,7 @@ let hubPool: Contract, identifierWhitelist: Contract; let owner: SignerWithAddress, other: SignerWithAddress; -describe.only("HubPool Admin functions", function () { +describe("HubPool Admin functions", function () { beforeEach(async function () { [owner, other] = await ethers.getSigners(); ({ weth, hubPool, usdc, mockAdapter, mockSpoke, identifierWhitelist } = await hubPoolFixture());