From 728d009523e384776867e3f44edd76e6dc8fe864 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 14 Mar 2022 20:00:26 -0400 Subject: [PATCH 1/4] improve: validate function inputs --- contracts/HubPool.sol | 1 + contracts/MerkleLib.sol | 12 ++++++------ contracts/Polygon_SpokePool.sol | 6 +----- contracts/SpokePool.sol | 6 ++---- contracts/test/MerkleLibTest.sol | 4 ++-- test/HubPool.ProtocolFees.ts | 5 +++++ test/MerkleLib.Claims.ts | 2 +- test/SpokePool.Relay.ts | 6 ++++++ 8 files changed, 24 insertions(+), 18 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index c10d7fb4a..cc36d0ae6 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -299,6 +299,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { public override onlyOwner + noActiveRequests { require(newProtocolFeeCapturePct <= 1e18, "Bad protocolFeeCapturePct"); protocolFeeCaptureAddress = newProtocolFeeCaptureAddress; diff --git a/contracts/MerkleLib.sol b/contracts/MerkleLib.sol index d7f9ffa42..85666ca8b 100644 --- a/contracts/MerkleLib.sol +++ b/contracts/MerkleLib.sol @@ -82,21 +82,21 @@ library MerkleLib { /** * @notice Tests whether a claim is contained within a 1D claimedBitMap mapping. * @param claimedBitMap a simple uint256 value, encoding a 1D bitmap. - * @param index the index to check in the bitmap. - \* @return bool indicating if the index within the claimedBitMap has been marked as claimed. + * @param index the index to check in the bitmap. Uint8 type enforces that index can't be > 255. + * @return bool indicating if the index within the claimedBitMap has been marked as claimed. */ - function isClaimed1D(uint256 claimedBitMap, uint256 index) internal pure returns (bool) { + function isClaimed1D(uint256 claimedBitMap, uint8 index) internal pure returns (bool) { uint256 mask = (1 << index); return claimedBitMap & mask == mask; } /** * @notice Marks an index in a claimedBitMap as claimed. - * @param claimedBitMap a simple uint256 mapping in storage used as a bitmap. + * @param claimedBitMap a simple uint256 mapping in storage used as a bitmap. Uint8 type enforces that index + * can't be > 255. * @param index the index to mark in the bitmap. */ - function setClaimed1D(uint256 claimedBitMap, uint256 index) internal pure returns (uint256) { - require(index <= 255, "Index out of bounds"); + function setClaimed1D(uint256 claimedBitMap, uint8 index) internal pure returns (uint256) { return claimedBitMap | (1 << index % 256); } } diff --git a/contracts/Polygon_SpokePool.sol b/contracts/Polygon_SpokePool.sol index 142456afb..7d943a119 100644 --- a/contracts/Polygon_SpokePool.sol +++ b/contracts/Polygon_SpokePool.sol @@ -134,11 +134,7 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool { ); // Note: WETH is WMATIC on matic, so this tells the tokenbridger that this is an unwrappable native token. - polygonTokenBridger.send( - PolygonIERC20(relayerRefundLeaf.l2TokenAddress), - relayerRefundLeaf.amountToReturn, - address(weth) == relayerRefundLeaf.l2TokenAddress - ); + polygonTokenBridger.send(PolygonIERC20(relayerRefundLeaf.l2TokenAddress), relayerRefundLeaf.amountToReturn); emit PolygonTokensBridged(relayerRefundLeaf.l2TokenAddress, address(this), relayerRefundLeaf.amountToReturn); } diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol index e31c73412..c42cec6f8 100644 --- a/contracts/SpokePool.sol +++ b/contracts/SpokePool.sol @@ -41,9 +41,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall // instruct this contract to wrap ETH when depositing. WETH9 public weth; - // Timestamp when contract was constructed. Relays cannot have a quote time before this. - uint32 public deploymentTime; - // Any deposit quote times greater than or less than this value to the current contract time is blocked. Forces // caller to use an approximately "current" realized fee. Defaults to 10 minutes. uint32 public depositQuoteTimeBuffer = 600; @@ -152,7 +149,6 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall ) Testable(timerAddress) { _setCrossDomainAdmin(_crossDomainAdmin); _setHubPool(_hubPool); - deploymentTime = uint32(getCurrentTime()); weth = WETH9(_wethAddress); } @@ -333,6 +329,8 @@ abstract contract SpokePool is SpokePoolInterface, Testable, Lockable, MultiCall uint32 depositId, bytes memory depositorSignature ) public override nonReentrant { + require(newRelayerFeePct < 0.5e18, "invalid relayer fee"); + _verifyUpdateRelayerFeeMessage(depositor, chainId(), newRelayerFeePct, depositId, depositorSignature); // Assuming the above checks passed, a relayer can take the signature and the updated relayer fee information diff --git a/contracts/test/MerkleLibTest.sol b/contracts/test/MerkleLibTest.sol index de8c7e3aa..a1e436de2 100644 --- a/contracts/test/MerkleLibTest.sol +++ b/contracts/test/MerkleLibTest.sol @@ -45,11 +45,11 @@ contract MerkleLibTest { MerkleLib.setClaimed(claimedBitMap, index); } - function isClaimed1D(uint256 index) public view returns (bool) { + function isClaimed1D(uint8 index) public view returns (bool) { return MerkleLib.isClaimed1D(claimedBitMap1D, index); } - function setClaimed1D(uint256 index) public { + function setClaimed1D(uint8 index) public { claimedBitMap1D = MerkleLib.setClaimed1D(claimedBitMap1D, index); } } diff --git a/test/HubPool.ProtocolFees.ts b/test/HubPool.ProtocolFees.ts index c3bbd35b1..256ca0931 100644 --- a/test/HubPool.ProtocolFees.ts +++ b/test/HubPool.ProtocolFees.ts @@ -27,6 +27,11 @@ describe("HubPool Protocol fees", function () { await expect(hubPool.connect(liquidityProvider).setProtocolFeeCapture(liquidityProvider.address, toWei("0.1"))).to .be.reverted; }); + it("Can not change the protocol fee capture during a pending refund", async function () { + const { tree } = await constructSingleChainTree(weth.address); + await hubPool.connect(dataWorker).proposeRootBundle([3117], 1, tree.getHexRoot(), mockTreeRoot, mockTreeRoot); + await expect(hubPool.connect(owner).setProtocolFeeCapture(liquidityProvider.address, toWei("0.1"))).to.be.reverted; + }); it("Can change protocol fee capture settings", async function () { expect(await hubPool.callStatic.protocolFeeCaptureAddress()).to.equal(owner.address); expect(await hubPool.callStatic.protocolFeeCapturePct()).to.equal(initialProtocolFeeCapturePct); diff --git a/test/MerkleLib.Claims.ts b/test/MerkleLib.Claims.ts index 74cc8388c..5ab9b2f8b 100644 --- a/test/MerkleLib.Claims.ts +++ b/test/MerkleLib.Claims.ts @@ -60,7 +60,7 @@ describe("MerkleLib Claims", async function () { // Setting right at the max should revert. await expect(merkleLibTest.setClaimed1D(256)).to.be.reverted; - expect(await merkleLibTest.isClaimed1D(255)).to.equal(false); + await expect(merkleLibTest.isClaimed1D(256)).to.be.reverted; // Should be able to set right below the max. expect(await merkleLibTest.isClaimed1D(255)).to.equal(false); diff --git a/test/SpokePool.Relay.ts b/test/SpokePool.Relay.ts index abc85e033..7f4f21cc6 100644 --- a/test/SpokePool.Relay.ts +++ b/test/SpokePool.Relay.ts @@ -176,6 +176,12 @@ describe("SpokePool Relayer Logic", async function () { spokePoolChainId.toString(), depositor ); + + // Cannot set new relayer fee pct >= 50% + await expect( + spokePool.connect(relayer).speedUpDeposit(depositor.address, toWei("0.5"), consts.firstDepositId, signature) + ).to.be.revertedWith("invalid relayer fee"); + await expect( spokePool .connect(relayer) From f0fa065b052bc20f505967c5ffef4d54b8372221 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 14 Mar 2022 20:08:12 -0400 Subject: [PATCH 2/4] Update Polygon_SpokePool.sol --- contracts/Polygon_SpokePool.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/contracts/Polygon_SpokePool.sol b/contracts/Polygon_SpokePool.sol index 7d943a119..142456afb 100644 --- a/contracts/Polygon_SpokePool.sol +++ b/contracts/Polygon_SpokePool.sol @@ -134,7 +134,11 @@ contract Polygon_SpokePool is IFxMessageProcessor, SpokePool { ); // Note: WETH is WMATIC on matic, so this tells the tokenbridger that this is an unwrappable native token. - polygonTokenBridger.send(PolygonIERC20(relayerRefundLeaf.l2TokenAddress), relayerRefundLeaf.amountToReturn); + polygonTokenBridger.send( + PolygonIERC20(relayerRefundLeaf.l2TokenAddress), + relayerRefundLeaf.amountToReturn, + address(weth) == relayerRefundLeaf.l2TokenAddress + ); emit PolygonTokensBridged(relayerRefundLeaf.l2TokenAddress, address(this), relayerRefundLeaf.amountToReturn); } From f89200b639be44660dc01b0d92476d039a209095 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 15 Mar 2022 10:01:51 -0400 Subject: [PATCH 3/4] add validation for setProtocolFeeCapture and spokePool address Signed-off-by: nicholaspai --- contracts/HubPool.sol | 70 ++++++++++++++++--------------- contracts/PolygonTokenBridger.sol | 6 +-- test/HubPool.Admin.ts | 16 +++++++ test/HubPool.ProtocolFees.ts | 5 +++ 4 files changed, 61 insertions(+), 36 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index cc36d0ae6..9e2f6751b 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -5,7 +5,6 @@ import "./MerkleLib.sol"; import "./HubPoolInterface.sol"; import "./Lockable.sol"; -import "./interfaces/AdapterInterface.sol"; import "./interfaces/LpTokenFactoryInterface.sol"; import "./interfaces/WETH9.sol"; @@ -99,7 +98,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // Heler contracts to facilitate cross chain actions between HubPool and SpokePool for a specific network. struct CrossChainContract { - AdapterInterface adapter; + address adapter; address spokePool; } // Mapping of chainId to the associated adapter and spokePool contracts. @@ -302,6 +301,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { noActiveRequests { require(newProtocolFeeCapturePct <= 1e18, "Bad protocolFeeCapturePct"); + require(newProtocolFeeCaptureAddress != address(0), "Bad protocolFeeCaptureAddress"); protocolFeeCaptureAddress = newProtocolFeeCaptureAddress; protocolFeeCapturePct = newProtocolFeeCapturePct; emit ProtocolFeeCaptureSet(newProtocolFeeCaptureAddress, newProtocolFeeCapturePct); @@ -366,7 +366,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { address adapter, address spokePool ) public override onlyOwner { - crossChainContracts[l2ChainId] = CrossChainContract(AdapterInterface(adapter), spokePool); + crossChainContracts[l2ChainId] = CrossChainContract(adapter, spokePool); emit CrossChainContractsSet(l2ChainId, adapter, spokePool); } @@ -633,9 +633,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { "Bad Proof" ); - // Before interacting with a particular chain's adapter, ensure that the adapter is set. - require(address(crossChainContracts[chainId].adapter) != address(0), "No adapter for chain"); - // Make sure SpokePool address is initialized since _sendTokensToChainAndUpdatePooledTokenTrackers() will not // revert if its accidentally set to address(0). We don't make the same check on the adapter for this // chainId because the internal method's delegatecall() to the adapter will revert if its address is set @@ -649,8 +646,33 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // Decrement the unclaimedPoolRebalanceLeafCount. rootBundleProposal.unclaimedPoolRebalanceLeafCount--; - _sendTokensToChainAndUpdatePooledTokenTrackers(spokePool, chainId, l1Tokens, netSendAmounts, bundleLpFees); - _relayRootBundleToSpokePool(spokePool, chainId); + // Relay each L1 token to destination chain. + // Note: We don't check that the adapter is initialized since if its the zero address or invalid otherwise, + // then the delegate call should revert. + address adapter = crossChainContracts[chainId].adapter; + _sendTokensToChainAndUpdatePooledTokenTrackers( + adapter, + spokePool, + chainId, + l1Tokens, + netSendAmounts, + bundleLpFees + ); + + // Relay root bundles to spoke pool on destination chain by + // performing delegatecall to use the adapter's code with this contract's context. + (bool success, ) = adapter.delegatecall( + abi.encodeWithSignature( + "relayMessage(address,bytes)", + spokePool, // target. This should be the spokePool on the L2. + abi.encodeWithSignature( + "relayRootBundle(bytes32,bytes32)", + rootBundleProposal.relayerRefundRoot, + rootBundleProposal.slowRelayRoot + ) // message + ) + ); + require(success, "delegatecall failed"); // Transfer the bondAmount to back to the proposer, if this the last executed leaf. Only sending this once all // leafs have been executed acts to force the data worker to execute all bundles or they wont receive their bond. @@ -852,14 +874,13 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { // Note this method does a lot and wraps together the sending of tokens and updating the pooled token trackers. This // is done as a gas saving so we don't need to iterate over the l1Tokens multiple times. function _sendTokensToChainAndUpdatePooledTokenTrackers( + address adapter, address spokePool, uint256 chainId, address[] memory l1Tokens, int256[] memory netSendAmounts, uint256[] memory bundleLpFees ) internal { - AdapterInterface adapter = crossChainContracts[chainId].adapter; - for (uint32 i = 0; i < l1Tokens.length; i++) { address l1Token = l1Tokens[i]; // Validate the L1 -> L2 token route is whitelisted. If it is not then the output of the bridging action @@ -872,7 +893,7 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { if (netSendAmounts[i] > 0) { // Perform delegatecall to use the adapter's code with this contract's context. Opt for delegatecall's // complexity in exchange for lower gas costs. - (bool success, ) = address(adapter).delegatecall( + (bool success, ) = adapter.delegatecall( abi.encodeWithSignature( "relayTokens(address,address,uint256,address)", l1Token, // l1Token. @@ -893,24 +914,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } } - function _relayRootBundleToSpokePool(address spokePool, uint256 chainId) internal { - AdapterInterface adapter = crossChainContracts[chainId].adapter; - - // Perform delegatecall to use the adapter's code with this contract's context. - (bool success, ) = address(adapter).delegatecall( - abi.encodeWithSignature( - "relayMessage(address,bytes)", - spokePool, // target. This should be the spokePool on the L2. - abi.encodeWithSignature( - "relayRootBundle(bytes32,bytes32)", - rootBundleProposal.relayerRefundRoot, - rootBundleProposal.slowRelayRoot - ) // message - ) - ); - require(success, "delegatecall failed"); - } - function _exchangeRateCurrent(address l1Token) internal returns (uint256) { PooledToken storage pooledToken = pooledTokens[l1Token]; // Note this is storage so the state can be modified. uint256 lpTokenTotalSupply = IERC20(pooledToken.lpToken).totalSupply(); @@ -1003,14 +1006,15 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { } function _relaySpokePoolAdminFunction(uint256 chainId, bytes memory functionData) internal { - AdapterInterface adapter = crossChainContracts[chainId].adapter; - require(address(adapter) != address(0), "Adapter not initialized"); + address adapter = crossChainContracts[chainId].adapter; + address spokePool = crossChainContracts[chainId].spokePool; + require(spokePool != address(0), "SpokePool not initialized"); // Perform delegatecall to use the adapter's code with this contract's context. - (bool success, ) = address(adapter).delegatecall( + (bool success, ) = adapter.delegatecall( abi.encodeWithSignature( "relayMessage(address,bytes)", - crossChainContracts[chainId].spokePool, // target. This should be the spokePool on the L2. + spokePool, // target. This should be the spokePool on the L2. functionData ) ); diff --git a/contracts/PolygonTokenBridger.sol b/contracts/PolygonTokenBridger.sol index a9250c503..c8f3cbc3d 100644 --- a/contracts/PolygonTokenBridger.sol +++ b/contracts/PolygonTokenBridger.sol @@ -54,12 +54,12 @@ contract PolygonTokenBridger is Lockable { * @notice Called by Polygon SpokePool to send tokens over bridge to contract with the same address as this. * @param token Token to bridge. * @param amount Amount to bridge. - * @param isMatic True if token is MATIC. + * @param isWrappedMatic True if token is WMATIC. */ function send( PolygonIERC20 token, uint256 amount, - bool isMatic + bool isWrappedMatic ) public nonReentrant { token.safeTransferFrom(msg.sender, address(this), amount); @@ -67,7 +67,7 @@ contract PolygonTokenBridger is Lockable { token.withdraw(amount); // This takes the token that was withdrawn and calls withdraw on the "native" ERC20. - if (isMatic) maticToken.withdraw{ value: amount }(amount); + if (isWrappedMatic) maticToken.withdraw{ value: amount }(amount); } /** diff --git a/test/HubPool.Admin.ts b/test/HubPool.Admin.ts index a718e591e..23ce84a85 100644 --- a/test/HubPool.Admin.ts +++ b/test/HubPool.Admin.ts @@ -47,6 +47,22 @@ describe("HubPool Admin functions", function () { hubPool.connect(other).setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address) ).to.be.reverted; }); + it("Only owner can relay spoke pool admin message", async function () { + const functionData = mockSpoke.interface.encodeFunctionData("setEnableRoute", [ + weth.address, + destinationChainId, + false, + ]); + await expect(hubPool.connect(other).relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.reverted; + + // Cannot relay admin function if spoke pool is set to zero address. + await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, ZERO_ADDRESS); + await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)).to.be.reverted; + await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address); + await expect(hubPool.relaySpokePoolAdminFunction(destinationChainId, functionData)) + .to.emit(hubPool, "SpokePoolAdminFunctionTriggered") + .withArgs(destinationChainId, functionData); + }); it("Only owner can whitelist route for deposits and rebalances", async function () { await hubPool.setCrossChainContracts(destinationChainId, mockAdapter.address, mockSpoke.address); await expect( diff --git a/test/HubPool.ProtocolFees.ts b/test/HubPool.ProtocolFees.ts index 256ca0931..c550aed2e 100644 --- a/test/HubPool.ProtocolFees.ts +++ b/test/HubPool.ProtocolFees.ts @@ -2,6 +2,7 @@ import { toWei, toBNWei, SignerWithAddress, seedWallet, expect, Contract, ethers import { mockTreeRoot, finalFee, bondAmount, amountToLp, refundProposalLiveness } from "./constants"; import { hubPoolFixture, enableTokensForLP } from "./fixtures/HubPool.Fixture"; import { constructSingleChainTree } from "./MerkleLib.utils"; +import { ZERO_ADDRESS } from "@uma/common"; let hubPool: Contract, weth: Contract, timer: Contract; let owner: SignerWithAddress, dataWorker: SignerWithAddress, liquidityProvider: SignerWithAddress; @@ -36,6 +37,10 @@ describe("HubPool Protocol fees", function () { expect(await hubPool.callStatic.protocolFeeCaptureAddress()).to.equal(owner.address); expect(await hubPool.callStatic.protocolFeeCapturePct()).to.equal(initialProtocolFeeCapturePct); const newPct = toWei("0.1"); + + // Can't set to 0 address + await expect(hubPool.connect(owner).setProtocolFeeCapture(ZERO_ADDRESS, newPct)).to.be.reverted; + await hubPool.connect(owner).setProtocolFeeCapture(liquidityProvider.address, newPct); expect(await hubPool.callStatic.protocolFeeCaptureAddress()).to.equal(liquidityProvider.address); expect(await hubPool.callStatic.protocolFeeCapturePct()).to.equal(newPct); From 4fe96074dff7b7b40a60139d54dfe6e99680fdef Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Tue, 15 Mar 2022 11:04:52 -0400 Subject: [PATCH 4/4] remove noActiveRequests from setProtocolFeeCapture --- contracts/HubPool.sol | 1 - test/HubPool.ProtocolFees.ts | 5 ----- 2 files changed, 6 deletions(-) diff --git a/contracts/HubPool.sol b/contracts/HubPool.sol index 9e2f6751b..afa4f26dd 100644 --- a/contracts/HubPool.sol +++ b/contracts/HubPool.sol @@ -298,7 +298,6 @@ contract HubPool is HubPoolInterface, Testable, Lockable, MultiCaller, Ownable { public override onlyOwner - noActiveRequests { require(newProtocolFeeCapturePct <= 1e18, "Bad protocolFeeCapturePct"); require(newProtocolFeeCaptureAddress != address(0), "Bad protocolFeeCaptureAddress"); diff --git a/test/HubPool.ProtocolFees.ts b/test/HubPool.ProtocolFees.ts index c550aed2e..ea7444f1f 100644 --- a/test/HubPool.ProtocolFees.ts +++ b/test/HubPool.ProtocolFees.ts @@ -28,11 +28,6 @@ describe("HubPool Protocol fees", function () { await expect(hubPool.connect(liquidityProvider).setProtocolFeeCapture(liquidityProvider.address, toWei("0.1"))).to .be.reverted; }); - it("Can not change the protocol fee capture during a pending refund", async function () { - const { tree } = await constructSingleChainTree(weth.address); - await hubPool.connect(dataWorker).proposeRootBundle([3117], 1, tree.getHexRoot(), mockTreeRoot, mockTreeRoot); - await expect(hubPool.connect(owner).setProtocolFeeCapture(liquidityProvider.address, toWei("0.1"))).to.be.reverted; - }); it("Can change protocol fee capture settings", async function () { expect(await hubPool.callStatic.protocolFeeCaptureAddress()).to.equal(owner.address); expect(await hubPool.callStatic.protocolFeeCapturePct()).to.equal(initialProtocolFeeCapturePct);