From b911329eaacb9e1a7b6a70b69143800bc7e41be2 Mon Sep 17 00:00:00 2001 From: Facu Spagnuolo Date: Wed, 25 Sep 2024 17:32:00 -0300 Subject: [PATCH] Tasks: Fix Hop bridger task when bridging to L1 (#164) Co-authored-by: lgalende <63872655+lgalende@users.noreply.github.com> --- .../tasks/contracts/bridge/HopBridger.sol | 12 +- .../interfaces/bridge/IHopBridger.sol | 5 - packages/tasks/test/bridge/HopBridger.test.ts | 342 ++++++++++-------- 3 files changed, 190 insertions(+), 169 deletions(-) diff --git a/packages/tasks/contracts/bridge/HopBridger.sol b/packages/tasks/contracts/bridge/HopBridger.sol index a187b0a1..9f035579 100644 --- a/packages/tasks/contracts/bridge/HopBridger.sol +++ b/packages/tasks/contracts/bridge/HopBridger.sol @@ -27,6 +27,9 @@ import '../interfaces/bridge/IHopBridger.sol'; contract HopBridger is IHopBridger, BaseBridgeTask { using FixedPoint for uint256; + // Ethereum mainnet chain ID = 1 + uint256 private constant MAINNET_CHAIN_ID = 1; + // Execution type for relayers bytes32 public constant override EXECUTION_TYPE = keccak256('HOP_BRIDGER'); @@ -97,7 +100,7 @@ contract HopBridger is IHopBridger, BaseBridgeTask { } /** - * @dev Sets the max deadline + * @dev Sets the max deadline. Only used when bridging to L2. * @param newMaxDeadline New max deadline to be set */ function setMaxDeadline(uint256 newMaxDeadline) external override authP(authParams(newMaxDeadline)) { @@ -128,8 +131,8 @@ contract HopBridger is IHopBridger, BaseBridgeTask { if (amount == 0) amount = getTaskAmount(token); _beforeHopBridger(token, amount, slippage, fee); - uint256 amountAfterFees = amount - fee; - uint256 minAmountOut = amountAfterFees.mulUp(FixedPoint.ONE - slippage); + uint256 destinationChain = getDestinationChain(token); + uint256 minAmountOut = (amount - fee).mulUp(FixedPoint.ONE - slippage); bytes memory connectorData = abi.encodeWithSelector( IHopBridgeConnector.execute.selector, getDestinationChain(token), @@ -138,7 +141,7 @@ contract HopBridger is IHopBridger, BaseBridgeTask { minAmountOut, recipient, tokenHopEntrypoint[token], - block.timestamp + maxDeadline, + destinationChain == MAINNET_CHAIN_ID ? 0 : (block.timestamp + maxDeadline), relayer, fee ); @@ -174,7 +177,6 @@ contract HopBridger is IHopBridger, BaseBridgeTask { * @dev Sets the max deadline */ function _setMaxDeadline(uint256 _maxDeadline) internal { - if (_maxDeadline == 0) revert TaskMaxDeadlineZero(); maxDeadline = _maxDeadline; emit MaxDeadlineSet(_maxDeadline); } diff --git a/packages/tasks/contracts/interfaces/bridge/IHopBridger.sol b/packages/tasks/contracts/interfaces/bridge/IHopBridger.sol index b07a3cb1..b55c0d26 100644 --- a/packages/tasks/contracts/interfaces/bridge/IHopBridger.sol +++ b/packages/tasks/contracts/interfaces/bridge/IHopBridger.sol @@ -20,11 +20,6 @@ import './IBaseBridgeTask.sol'; * @dev Hop bridger task interface */ interface IHopBridger is IBaseBridgeTask { - /** - * @dev The max deadline is zero - */ - error TaskMaxDeadlineZero(); - /** * @dev The Hop entrypoint is zero */ diff --git a/packages/tasks/test/bridge/HopBridger.test.ts b/packages/tasks/test/bridge/HopBridger.test.ts index 95a204f3..dd755201 100644 --- a/packages/tasks/test/bridge/HopBridger.test.ts +++ b/packages/tasks/test/bridge/HopBridger.test.ts @@ -113,9 +113,7 @@ describe('HopBridger', () => { task = task.connect(owner) }) - context('when the deadline is not zero', () => { - const deadline = 60 * 60 - + const itSetsTheMaxDeadlineProperly = (deadline: number) => { it('sets the max deadline', async () => { await task.setMaxDeadline(deadline) @@ -127,14 +125,18 @@ describe('HopBridger', () => { await assertEvent(tx, 'MaxDeadlineSet', { maxDeadline: deadline }) }) + } + + context('when the deadline is not zero', () => { + const deadline = 60 * 60 + + itSetsTheMaxDeadlineProperly(deadline) }) context('when the deadline is zero', () => { const deadline = 0 - it('reverts', async () => { - await expect(task.setMaxDeadline(deadline)).to.be.revertedWith('TaskMaxDeadlineZero') - }) + itSetsTheMaxDeadlineProperly(deadline) }) }) @@ -272,217 +274,239 @@ describe('HopBridger', () => { const slippage = fp(0.05) context('when the destination chain was set', () => { - const chainId = 1 - - beforeEach('set destination chain ID', async () => { - const setDefaultDestinationChainRole = task.interface.getSighash('setDefaultDestinationChain') - await authorizer.connect(owner).authorize(owner.address, task.address, setDefaultDestinationChainRole, []) - await task.connect(owner).setDefaultDestinationChain(chainId) - }) - - context('when the given token is allowed', () => { - context('when the current balance passes the threshold', () => { - const threshold = amount + const itExecutesProperlyForChain = (chainId: number) => { + beforeEach('set destination chain ID', async () => { + const setDefaultDestinationChainRole = task.interface.getSighash('setDefaultDestinationChain') + await authorizer + .connect(owner) + .authorize(owner.address, task.address, setDefaultDestinationChainRole, []) + await task.connect(owner).setDefaultDestinationChain(chainId) + }) - beforeEach('set threshold', async () => { - const setDefaultTokenThresholdRole = task.interface.getSighash('setDefaultTokenThreshold') - await authorizer - .connect(owner) - .authorize(owner.address, task.address, setDefaultTokenThresholdRole, []) - await task.connect(owner).setDefaultTokenThreshold(token.address, threshold, 0) - }) + context('when the given token is allowed', () => { + context('when the current balance passes the threshold', () => { + const threshold = amount - beforeEach('fund smart vault', async () => { - await token.mint(smartVault.address, amount) - }) + beforeEach('set threshold', async () => { + const setDefaultTokenThresholdRole = task.interface.getSighash('setDefaultTokenThreshold') + await authorizer + .connect(owner) + .authorize(owner.address, task.address, setDefaultTokenThresholdRole, []) + await task.connect(owner).setDefaultTokenThreshold(token.address, threshold, 0) + }) - beforeEach('set token hop entrypoint', async () => { - const setTokenHopEntrypointRole = task.interface.getSighash('setTokenHopEntrypoint') - await authorizer.connect(owner).authorize(owner.address, task.address, setTokenHopEntrypointRole, []) - await task.connect(owner).setTokenHopEntrypoint(token.address, entrypoint.address) - }) + beforeEach('fund smart vault', async () => { + await token.mint(smartVault.address, amount) + }) - context('when the slippage is below the limit', () => { - beforeEach('set max slippage', async () => { - const setDefaultMaxSlippageRole = task.interface.getSighash('setDefaultMaxSlippage') + beforeEach('set token hop entrypoint', async () => { + const setTokenHopEntrypointRole = task.interface.getSighash('setTokenHopEntrypoint') await authorizer .connect(owner) - .authorize(owner.address, task.address, setDefaultMaxSlippageRole, []) - await task.connect(owner).setDefaultMaxSlippage(slippage) + .authorize(owner.address, task.address, setTokenHopEntrypointRole, []) + await task.connect(owner).setTokenHopEntrypoint(token.address, entrypoint.address) }) - const itExecutesTheTaskProperly = (requestedAmount: BigNumberish, fee: BigNumberish) => { - it('executes the expected connector', async () => { - const tx = await task.call(token.address, requestedAmount, slippage, fee) - - const deadline = (await currentTimestamp()).add(MAX_UINT256.div(10)) - const amountAfterFees = amount.sub(fee) - const minAmountOut = amountAfterFees.mul(fp(1).sub(slippage)).div(fp(1)) - - const connectorData = connector.interface.encodeFunctionData('execute', [ - chainId, - token.address, - amount, - minAmountOut, - smartVault.address, - entrypoint.address, - deadline, - relayer.address, - fee, - ]) - await assertIndirectEvent(tx, smartVault.interface, 'Executed', { - connector, - data: connectorData, - }) + context('when the slippage is below the limit', () => { + beforeEach('set max slippage', async () => { + const setDefaultMaxSlippageRole = task.interface.getSighash('setDefaultMaxSlippage') + await authorizer + .connect(owner) + .authorize(owner.address, task.address, setDefaultMaxSlippageRole, []) + await task.connect(owner).setDefaultMaxSlippage(slippage) + }) + + const itExecutesTheTaskProperly = (requestedAmount: BigNumberish, fee: BigNumberish) => { + it('executes the expected connector', async () => { + const tx = await task.call(token.address, requestedAmount, slippage, fee) + + const deadline = chainId == 1 ? 0 : (await currentTimestamp()).add(MAX_UINT256.div(10)) + const amountAfterFees = amount.sub(fee) + const minAmountOut = amountAfterFees.mul(fp(1).sub(slippage)).div(fp(1)) + + const connectorData = connector.interface.encodeFunctionData('execute', [ + chainId, + token.address, + amount, + minAmountOut, + smartVault.address, + entrypoint.address, + deadline, + relayer.address, + fee, + ]) + await assertIndirectEvent(tx, smartVault.interface, 'Executed', { + connector, + data: connectorData, + }) - await assertIndirectEvent(tx, connector.interface, 'LogExecute', { - chainId, - token, - amount, - minAmountOut, - recipient: smartVault, - bridge: entrypoint, - deadline, - relayer, - fee, + await assertIndirectEvent(tx, connector.interface, 'LogExecute', { + chainId, + token, + amount, + minAmountOut, + recipient: smartVault, + bridge: entrypoint, + deadline, + relayer, + fee, + }) }) - }) - it('emits an Executed event', async () => { - const tx = await task.call(token.address, requestedAmount, slippage, fee) + it('emits an Executed event', async () => { + const tx = await task.call(token.address, requestedAmount, slippage, fee) - await assertEvent(tx, 'Executed') - }) - } + await assertEvent(tx, 'Executed') + }) + } - context('when the max fee is set', () => { - beforeEach('set max fee', async () => { - const setDefaultMaxFeeRole = task.interface.getSighash('setDefaultMaxFee') - await authorizer.connect(owner).authorize(owner.address, task.address, setDefaultMaxFeeRole, []) - await task.connect(owner).setDefaultMaxFee(token.address, fee) - }) + context('when the max fee is set', () => { + beforeEach('set max fee', async () => { + const setDefaultMaxFeeRole = task.interface.getSighash('setDefaultMaxFee') + await authorizer.connect(owner).authorize(owner.address, task.address, setDefaultMaxFeeRole, []) + await task.connect(owner).setDefaultMaxFee(token.address, fee) + }) - context('when the given fee is below the limit', () => { - context('without balance connectors', () => { - const requestedAmount = amount + context('when the given fee is below the limit', () => { + context('without balance connectors', () => { + const requestedAmount = amount - itExecutesTheTaskProperly(requestedAmount, fee) + itExecutesTheTaskProperly(requestedAmount, fee) - it('does not update any balance connectors', async () => { - const tx = await task.call(token.address, requestedAmount, slippage, fee) + it('does not update any balance connectors', async () => { + const tx = await task.call(token.address, requestedAmount, slippage, fee) - await assertNoEvent(tx, 'BalanceConnectorUpdated') + await assertNoEvent(tx, 'BalanceConnectorUpdated') + }) }) - }) - context('with balance connectors', () => { - const requestedAmount = 0 - const prevConnectorId = '0x0000000000000000000000000000000000000000000000000000000000000001' + context('with balance connectors', () => { + const requestedAmount = 0 + const prevConnectorId = '0x0000000000000000000000000000000000000000000000000000000000000001' - beforeEach('set balance connectors', async () => { - const setBalanceConnectorsRole = task.interface.getSighash('setBalanceConnectors') - await authorizer - .connect(owner) - .authorize(owner.address, task.address, setBalanceConnectorsRole, []) - await task.connect(owner).setBalanceConnectors(prevConnectorId, ZERO_BYTES32) - }) + beforeEach('set balance connectors', async () => { + const setBalanceConnectorsRole = task.interface.getSighash('setBalanceConnectors') + await authorizer + .connect(owner) + .authorize(owner.address, task.address, setBalanceConnectorsRole, []) + await task.connect(owner).setBalanceConnectors(prevConnectorId, ZERO_BYTES32) + }) - beforeEach('authorize task to update balance connectors', async () => { - const updateBalanceConnectorRole = smartVault.interface.getSighash('updateBalanceConnector') - await authorizer - .connect(owner) - .authorize(task.address, smartVault.address, updateBalanceConnectorRole, []) - }) + beforeEach('authorize task to update balance connectors', async () => { + const updateBalanceConnectorRole = smartVault.interface.getSighash('updateBalanceConnector') + await authorizer + .connect(owner) + .authorize(task.address, smartVault.address, updateBalanceConnectorRole, []) + }) - beforeEach('assign amount to previous balance connector', async () => { - const updateBalanceConnectorRole = smartVault.interface.getSighash('updateBalanceConnector') - await authorizer - .connect(owner) - .authorize(owner.address, smartVault.address, updateBalanceConnectorRole, []) - await smartVault - .connect(owner) - .updateBalanceConnector(prevConnectorId, token.address, amount, true) - }) + beforeEach('assign amount to previous balance connector', async () => { + const updateBalanceConnectorRole = smartVault.interface.getSighash('updateBalanceConnector') + await authorizer + .connect(owner) + .authorize(owner.address, smartVault.address, updateBalanceConnectorRole, []) + await smartVault + .connect(owner) + .updateBalanceConnector(prevConnectorId, token.address, amount, true) + }) - itExecutesTheTaskProperly(requestedAmount, fee) + itExecutesTheTaskProperly(requestedAmount, fee) - it('updates the balance connectors properly', async () => { - const tx = await task.call(token.address, amount, slippage, fee) + it('updates the balance connectors properly', async () => { + const tx = await task.call(token.address, amount, slippage, fee) - await assertIndirectEvent(tx, smartVault.interface, 'BalanceConnectorUpdated', { - id: prevConnectorId, - token, - amount: amount, - added: false, + await assertIndirectEvent(tx, smartVault.interface, 'BalanceConnectorUpdated', { + id: prevConnectorId, + token, + amount: amount, + added: false, + }) }) }) }) - }) - context('when the given fee is above the limit', () => { - const highFee = fee.add(1) + context('when the given fee is above the limit', () => { + const highFee = fee.add(1) - it('reverts', async () => { - await expect(task.call(token.address, amount, 0, highFee)).to.be.revertedWith('TaskFeeAboveMax') + it('reverts', async () => { + await expect(task.call(token.address, amount, 0, highFee)).to.be.revertedWith( + 'TaskFeeAboveMax' + ) + }) }) }) - }) - context('when the max fee is not set', () => { - context('when the given fee is zero', () => { - const fee = 0 + context('when the max fee is not set', () => { + context('when the given fee is zero', () => { + const fee = 0 - itExecutesTheTaskProperly(amount, fee) - }) + itExecutesTheTaskProperly(amount, fee) + }) - context('when the given fee is not zero', () => { - it('reverts', async () => { - await expect(task.call(token.address, amount, slippage, fee)).to.be.revertedWith( - 'TaskFeeAboveMax' - ) + context('when the given fee is not zero', () => { + it('reverts', async () => { + await expect(task.call(token.address, amount, slippage, fee)).to.be.revertedWith( + 'TaskFeeAboveMax' + ) + }) }) }) }) + + context('when the slippage is above the limit', () => { + it('reverts', async () => { + await expect(task.call(token.address, amount, slippage, 0)).to.be.revertedWith( + 'TaskSlippageAboveMax' + ) + }) + }) }) - context('when the slippage is above the limit', () => { + context('when the current balance does not pass the threshold', () => { + const threshold = amount.add(1) + + beforeEach('set threshold', async () => { + const setDefaultTokenThresholdRole = task.interface.getSighash('setDefaultTokenThreshold') + await authorizer + .connect(owner) + .authorize(owner.address, task.address, setDefaultTokenThresholdRole, []) + await task.connect(owner).setDefaultTokenThreshold(token.address, threshold, 0) + }) + it('reverts', async () => { - await expect(task.call(token.address, amount, slippage, 0)).to.be.revertedWith( - 'TaskSlippageAboveMax' + await expect(task.call(token.address, amount, slippage, fee)).to.be.revertedWith( + 'TaskTokenThresholdNotMet' ) }) }) }) - context('when the current balance does not pass the threshold', () => { - const threshold = amount.add(1) - - beforeEach('set threshold', async () => { - const setDefaultTokenThresholdRole = task.interface.getSighash('setDefaultTokenThreshold') + context('when the given token is not allowed', () => { + beforeEach('deny token', async () => { + const setTokensAcceptanceListRole = task.interface.getSighash('setTokensAcceptanceList') await authorizer .connect(owner) - .authorize(owner.address, task.address, setDefaultTokenThresholdRole, []) - await task.connect(owner).setDefaultTokenThreshold(token.address, threshold, 0) + .authorize(owner.address, task.address, setTokensAcceptanceListRole, []) + await task.connect(owner).setTokensAcceptanceList([token.address], [true]) }) it('reverts', async () => { await expect(task.call(token.address, amount, slippage, fee)).to.be.revertedWith( - 'TaskTokenThresholdNotMet' + 'TaskTokenNotAllowed' ) }) }) + } + + context('when bridging to L1', () => { + const chainId = 1 + + itExecutesProperlyForChain(chainId) }) - context('when the given token is not allowed', () => { - beforeEach('deny token', async () => { - const setTokensAcceptanceListRole = task.interface.getSighash('setTokensAcceptanceList') - await authorizer.connect(owner).authorize(owner.address, task.address, setTokensAcceptanceListRole, []) - await task.connect(owner).setTokensAcceptanceList([token.address], [true]) - }) + context('when bridging to an L2', () => { + const chainId = 137 - it('reverts', async () => { - await expect(task.call(token.address, amount, slippage, fee)).to.be.revertedWith('TaskTokenNotAllowed') - }) + itExecutesProperlyForChain(chainId) }) })