From c49b0e72c993e5b34318af9fe58ef1165f6aacd8 Mon Sep 17 00:00:00 2001 From: raulk Date: Tue, 19 Mar 2024 19:37:03 +0000 Subject: [PATCH] extras/axelar-token: improve error handling and add recovery paths. (#814) --- extras/axelar-token/.env.example | 41 +++++++--- extras/axelar-token/script/Deploy.s.sol | 3 +- extras/axelar-token/src/IpcTokenHandler.sol | 64 ++++++++------- extras/axelar-token/test/TestHandler.sol | 91 ++++++++++++++++++++- 4 files changed, 151 insertions(+), 48 deletions(-) diff --git a/extras/axelar-token/.env.example b/extras/axelar-token/.env.example index 18ee833aa..051e337cf 100644 --- a/extras/axelar-token/.env.example +++ b/extras/axelar-token/.env.example @@ -2,38 +2,53 @@ ## To change the network to use as origin and destination, modify these following environment variables. ## Other environment variables are prefixed with the network name, followed by two underscores. ## For example, for the Polygon Mumbai network, the private key environment variable is POLYGON_MUMBAI__PRIVATE_KEY -ORIGIN_NETWORK=POLYGON_MUMBAI # Specifies the origin network as Polygon Mumbai -DEST_NETWORK=FILECOIN_CALIBRATION # Specifies the destination network as Filecoin Calibration +# Specifies the origin network as Polygon Mumbai +ORIGIN_NETWORK=POLYGON_MUMBAI +# Specifies the destination network as Filecoin Calibration +DEST_NETWORK=FILECOIN_CALIBRATION ### ### Polygon Mumbai ######################### -POLYGON_MUMBAI__PRIVATE_KEY= # Private key for the Polygon Mumbai network -POLYGON_MUMBAI__RPC_URL=https://polygon-mumbai.gateway.tenderly.co # RPC URL for the Polygon Mumbai network +# Private key for the Polygon Mumbai network +POLYGON_MUMBAI__PRIVATE_KEY= +# RPC URL for the Polygon Mumbai network +POLYGON_MUMBAI__RPC_URL=https://polygon-mumbai.gateway.tenderly.co # Axelar config; see https://docs.axelar.dev/resources/contract-addresses/testnet -POLYGON_MUMBAI__AXELAR_ITS_ADDRESS=0xB5FB4BE02232B1bBA4dC8f81dc24C26980dE9e3C # Axelar ITS address for the Polygon Mumbai network -POLYGON_MUMBAI__AXELAR_CHAIN_NAME=Polygon # Axelar chain name for the Polygon Mumbai network +# Axelar ITS address for the Polygon Mumbai network +POLYGON_MUMBAI__AXELAR_ITS_ADDRESS=0xB5FB4BE02232B1bBA4dC8f81dc24C26980dE9e3C +# Axelar chain name for the Polygon Mumbai network +POLYGON_MUMBAI__AXELAR_CHAIN_NAME=Polygon ### ### Filecoin Calibration ######################### -FILECOIN_CALIBRATION__PRIVATE_KEY= # Private key for the Filecoin Calibration network -FILECOIN_CALIBRATION__RPC_URL=https://api.calibration.node.glif.io/rpc/v1 # RPC URL for the Filecoin Calibration network +# Private key for the Filecoin Calibration network +FILECOIN_CALIBRATION__PRIVATE_KEY= +# RPC URL for the Filecoin Calibration network +FILECOIN_CALIBRATION__RPC_URL=https://api.calibration.node.glif.io/rpc/v1 # Axelar config; see https://docs.axelar.dev/resources/contract-addresses/testnet -FILECOIN_CALIBRATION__AXELAR_ITS_ADDRESS=0xB5FB4BE02232B1bBA4dC8f81dc24C26980dE9e3C # Axelar ITS address for the Filecoin Calibration network -FILECOIN_CALIBRATION__AXELAR_CHAIN_NAME=filecoin-2 # Axelar chain name for the Filecoin Calibration network +# Axelar ITS address for the Filecoin Calibration network +FILECOIN_CALIBRATION__AXELAR_ITS_ADDRESS=0xB5FB4BE02232B1bBA4dC8f81dc24C26980dE9e3C +# Axelar chain name for the Filecoin Calibration network +FILECOIN_CALIBRATION__AXELAR_CHAIN_NAME=filecoin-2 # IPC -FILECOIN_CALIBRATION__IPC_GATEWAY_ADDRESS=0xdd0A8b2f247b8444AF716b14250939B57cdBe1E8 # IPC gateway address for the Filecoin Calibration network +# IPC gateway address for the Filecoin Calibration network +FILECOIN_CALIBRATION__IPC_GATEWAY_ADDRESS=0xdd0A8b2f247b8444AF716b14250939B57cdBe1E8 +# The admin address of the IpcTokenHandler, which is authorized to operate the treasury to recover funds. +FILECOIN_CALIBRATION__HANDLER_ADMIN_ADDRESS=0x ### ### Axelar token addresses ################################ # https://testnet.interchain.axelar.dev/polygon/0x0cc94E0b1056Ab7125DD820b406C0A4581CCec91 -POLYGON_MUMBAI__ORIGIN_TOKEN_ADDRESS=0x0cc94E0b1056Ab7125DD820b406C0A4581CCec91 # Origin token address for the Polygon Mumbai network -FILECOIN_CALIBRATION__REMOTE_TOKEN_ADDRESS=0x0cc94E0b1056Ab7125DD820b406C0A4581CCec91 # Remote token address for the Filecoin Calibration network +# Origin token address for the Polygon Mumbai network +POLYGON_MUMBAI__ORIGIN_TOKEN_ADDRESS=0x0cc94E0b1056Ab7125DD820b406C0A4581CCec91 +# Remote token address for the Filecoin Calibration network +FILECOIN_CALIBRATION__REMOTE_TOKEN_ADDRESS=0x0cc94E0b1056Ab7125DD820b406C0A4581CCec91 diff --git a/extras/axelar-token/script/Deploy.s.sol b/extras/axelar-token/script/Deploy.s.sol index f26b0ef57..c97ba266a 100644 --- a/extras/axelar-token/script/Deploy.s.sol +++ b/extras/axelar-token/script/Deploy.s.sol @@ -17,7 +17,8 @@ contract Deploy is Script { vm.startBroadcast(privateKey); IpcTokenHandler handler = new IpcTokenHandler({ axelarIts: vm.envAddress(string.concat(network, "__AXELAR_ITS_ADDRESS")), - ipcGateway: vm.envAddress(string.concat(network, "__IPC_GATEWAY_ADDRESS")) + ipcGateway: vm.envAddress(string.concat(network, "__IPC_GATEWAY_ADDRESS")), + admin: vm.envAddress(string.concat(network, "__HANDLER_ADMIN_ADDRESS")) }); vm.stopBroadcast(); diff --git a/extras/axelar-token/src/IpcTokenHandler.sol b/extras/axelar-token/src/IpcTokenHandler.sol index d9408b930..5ace67ba3 100644 --- a/extras/axelar-token/src/IpcTokenHandler.sol +++ b/extras/axelar-token/src/IpcTokenHandler.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.20; import { InterchainTokenExecutable } from '@axelar-network/interchain-token-service/executable/InterchainTokenExecutable.sol'; import { IERC20 } from "openzeppelin-contracts/interfaces/IERC20.sol"; +import { Ownable } from "openzeppelin-contracts/access/Ownable.sol"; import { SubnetID, SupplySource, SupplyKind } from "@ipc/src/structs/Subnet.sol"; import { FvmAddress } from "@ipc/src/structs/FvmAddress.sol"; import { IpcHandler } from "@ipc/sdk/IpcContract.sol"; @@ -23,7 +24,7 @@ interface SubnetActor { // IpcTokenSender via the Axelar ITS, receiving some token value to deposit into an IPC subnet (specified in the // incoming message). The IpcTokenHandler handles deposit failures by crediting the value back to the original // beneficiary, and making it available from them to withdraw() on the rootnet. -contract IpcTokenHandler is InterchainTokenExecutable, IpcHandler { +contract IpcTokenHandler is InterchainTokenExecutable, IpcHandler, Ownable { using FvmAddressHelper for address; using FvmAddressHelper for FvmAddress; using SubnetIDHelper for SubnetID; @@ -31,13 +32,12 @@ contract IpcTokenHandler is InterchainTokenExecutable, IpcHandler { error NothingToWithdraw(); - TokenFundedGateway public _ipcGateway; - mapping(address beneficiary => mapping(address token => uint256 value)) private _claims; - event SubnetFunded(SubnetID indexed subnet, address indexed recipient, uint256 value); event FundingFailed(SubnetID indexed subnet, address indexed recipient, uint256 value); - constructor(address axelarIts, address ipcGateway) InterchainTokenExecutable(axelarIts) { + TokenFundedGateway public _ipcGateway; + + constructor(address axelarIts, address ipcGateway, address admin) InterchainTokenExecutable(axelarIts) Ownable(admin) { _ipcGateway = TokenFundedGateway(ipcGateway); } @@ -52,18 +52,33 @@ contract IpcTokenHandler is InterchainTokenExecutable, IpcHandler { address tokenAddr, uint256 amount ) internal override { - (SubnetID memory subnet, address recipient) = abi.decode(data, (SubnetID, address)); - IERC20 token = IERC20(tokenAddr); require(token.balanceOf(address(this)) >= amount, "insufficient balance"); // Authorize the IPC gateway to spend these tokens on our behalf. - token.approve(address(_ipcGateway), amount); + token.safeIncreaseAllowance(address(_ipcGateway), amount); + + // Try to decode the payload. Note: Solidity does not support try/catch for abi.decode (or tryDecode), so + // this may fail if there's a bug in the sender (in which case funds can be retrieved through the admin path). + (SubnetID memory subnet, address recipient) = abi.decode(data, (SubnetID, address)); + + (bool success, ) = address(_ipcGateway).call( + abi.encodeWithSelector(TokenFundedGateway.fundWithToken.selector, subnet, recipient.from(), amount) + ); - // Fund the designated subnet via the IPC gateway. - _ipcGateway.fundWithToken(subnet, recipient.from(), amount); + if (!success) { + // Restore the original allowance. + token.safeDecreaseAllowance(address(_ipcGateway), amount); + + // Increase the allowance of the admin address so they can retrieve these otherwise lost tokens. + token.safeIncreaseAllowance(owner(), amount); + + // Emit a FundingFailed event. + emit FundingFailed(subnet, recipient, amount); + + return; + } - // Emit an event. emit SubnetFunded(subnet, recipient, amount); } @@ -78,18 +93,16 @@ contract IpcTokenHandler is InterchainTokenExecutable, IpcHandler { ResultMsg memory result = abi.decode(envelope.message, (ResultMsg)); if (result.outcome != OutcomeType.Ok) { - // Note: IPC only supports deploying subnets via our blessed registry, so we can trust the code behind - // the subnet actor. + // Verify that the subnet is indeed an ERC20 subnet. SupplySource memory supplySource = SubnetActor(envelope.from.subnetId.getAddress()).supplySource(); require(supplySource.kind == SupplyKind.ERC20, "expected ERC20 supply source"); + // Increase the allowance of the admin address so they can retrieve these otherwise lost tokens. + IERC20(supplySource.tokenAddress).safeIncreaseAllowance(owner(), envelope.value); + // Results will carry the original beneficiary in the 'from' address. address beneficiary = envelope.from.rawAddress.extractEvmAddress(); - // We credit the token funds to the beneficiary. The beneficiary will have to call withdraw() to pull the - // funds out on this network. - _claims[beneficiary][supplySource.tokenAddress] += envelope.value; - // Emit an event. emit FundingFailed(envelope.from.subnetId, beneficiary, envelope.value); } @@ -97,19 +110,10 @@ contract IpcTokenHandler is InterchainTokenExecutable, IpcHandler { return bytes(""); } - // @notice Withdraws all available balance for the specified token for the sender. - function withdraw(address token) external { - uint256 available = _claims[msg.sender][token]; - if (available == 0) { - revert NothingToWithdraw(); - } - - delete _claims[msg.sender][token]; - IERC20(token).safeTransfer(msg.sender, available); + // @notice The ultimate backstop in case the error-handling logic itself failed unexpectedly and we failed to + // increase the recovery allowances of the admin address. + function adminTokenIncreaseAllowance(address token, uint256 amount) external onlyOwner { + IERC20(token).safeIncreaseAllowance(owner(), amount); } - // @notice Queries the claim of a beneficiary over a particular token. - function getClaimFor(address beneficiary, address token) external view returns (uint256) { - return _claims[beneficiary][token]; - } } \ No newline at end of file diff --git a/extras/axelar-token/test/TestHandler.sol b/extras/axelar-token/test/TestHandler.sol index 7242f65a2..6728afd08 100644 --- a/extras/axelar-token/test/TestHandler.sol +++ b/extras/axelar-token/test/TestHandler.sol @@ -12,19 +12,21 @@ contract TestHandler is Test { function test_handler_Ok() public { address axelarIts = vm.addr(1); address ipcGateway = vm.addr(2); + address owner = vm.addr(3); DummyERC20 token = new DummyERC20("Test token", "TST", 10000); IpcTokenHandler handler = new IpcTokenHandler({ axelarIts: axelarIts, - ipcGateway: ipcGateway + ipcGateway: ipcGateway, + admin: owner }); address[] memory route = new address[](1); route[0] = 0x2a3eF0F414c626e51AFA2F29f3F7Be7a45C6DB09; + SubnetID memory subnet = SubnetID({ root: 314159, route: route }); address recipient = 0x6B505cdCCCA34aE8eea5D382aBaD40d2AfEa74ad; - SubnetID memory subnet = SubnetID({ root: 314159, route: route }); bytes memory params = abi.encode(subnet, recipient); token.transfer(address(handler), 1); @@ -36,10 +38,91 @@ contract TestHandler is Test { abi.encode("") ); handler.executeWithInterchainToken(bytes32(""), "", "", params, bytes32(""), address(token), 1); + + // the allowance of the gateway is still 1, because the call to fundWithToken was mocked and did not actually expend the allowance + // this is not what would happen in reality, but the assert gives us extra insight + require(token.allowance(address(handler), ipcGateway) == 1); } - // TODO test_handler_err_withdrawal (also test getClaims) + function test_handler_failGateway() public { + address axelarIts = vm.addr(1); + address ipcGateway = vm.addr(2); + address owner = vm.addr(3); + DummyERC20 token = new DummyERC20("Test token", "TST", 10000); + + IpcTokenHandler handler = new IpcTokenHandler({ + axelarIts: axelarIts, + ipcGateway: ipcGateway, + admin: owner + }); - // TODO test_handler_err_deposit (e.g. sending to a non-ERC20 subnet) + address[] memory route = new address[](1); + route[0] = 0x2a3eF0F414c626e51AFA2F29f3F7Be7a45C6DB09; + SubnetID memory subnet = SubnetID({ root: 314159, route: route }); + + address recipient = 0x6B505cdCCCA34aE8eea5D382aBaD40d2AfEa74ad; + + bytes memory params = abi.encode(subnet, recipient); + + token.transfer(address(handler), 1); + vm.startPrank(axelarIts); + + vm.expectEmit(); + emit IERC20.Approval(address(handler), address(ipcGateway), 1); + emit IERC20.Approval(address(handler), address(ipcGateway), 0); + emit IERC20.Approval(address(handler), address(owner), 1); + emit IpcTokenHandler.FundingFailed(subnet, recipient, 1); + + vm.mockCallRevert( + address(ipcGateway), + abi.encodeWithSelector(TokenFundedGateway.fundWithToken.selector, subnet, recipient.from(), 1), + abi.encode("ERROR") + ); + handler.executeWithInterchainToken(bytes32(""), "", "", params, bytes32(""), address(token), 1); + + // the allowance was accrued to the owner + require(token.allowance(address(handler), ipcGateway) == 0); + require(token.allowance(address(handler), owner) == 1); + } + + function test_handler_fail_unexpected() public { + address axelarIts = vm.addr(1); + address ipcGateway = vm.addr(2); + address owner = vm.addr(3); + DummyERC20 token = new DummyERC20("Test token", "TST", 10000); + + IpcTokenHandler handler = new IpcTokenHandler({ + axelarIts: axelarIts, + ipcGateway: ipcGateway, + admin: owner + }); + + // garbage + bytes memory params = abi.encode(1); + + token.transfer(address(handler), 4200); + vm.startPrank(axelarIts); + + // will revert due to garbage. + vm.expectRevert(); + handler.executeWithInterchainToken(bytes32(""), "", "", params, bytes32(""), address(token), 4200); + + // let's ensure we can recover the tokens + require(token.allowance(address(handler), owner) == 0); + + // this should revert when called by the previous pranked account (non-owner). + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, axelarIts)); + handler.adminTokenIncreaseAllowance(address(token), 4200); + + // now act like the owner. + vm.startPrank(owner); + + handler.adminTokenIncreaseAllowance(address(token), 4200); + require(token.allowance(address(handler), owner) == 4200); + + token.transferFrom(address(handler), owner, 4200); + require(token.allowance(address(handler), owner) == 0); + require(token.balanceOf(owner) == 4200); + } } \ No newline at end of file