From 69ec4b2c0bf275d4798f68211f788ded0cadb57b Mon Sep 17 00:00:00 2001 From: Agustin Velez Date: Tue, 22 Oct 2024 19:56:24 -0300 Subject: [PATCH 1/9] add dispatcher components --- .gitmodules | 5 +- foundry.toml | 1 + lib/openzeppelin-contracts | 1 + src/TransferUtils.sol | 26 + src/dispatcherComponents/AccessControl.sol | 247 +++++++++ src/dispatcherComponents/SweepTokens.sol | 38 ++ src/dispatcherComponents/Upgrade.sol | 68 +++ src/dispatcherComponents/ids.sol | 36 ++ src/testing/UpgradeTester.sol | 26 + test/dispatcherComponents/AccessControl.t.sol | 523 ++++++++++++++++++ test/dispatcherComponents/SweepTokens.t.sol | 52 ++ test/dispatcherComponents/Upgrade.t.sol | 136 +++++ .../dispatcherComponents/utils/Dispatcher.sol | 69 +++ .../utils/DispatcherTestBase.sol | 69 +++ 14 files changed, 1296 insertions(+), 1 deletion(-) create mode 160000 lib/openzeppelin-contracts create mode 100644 src/TransferUtils.sol create mode 100644 src/dispatcherComponents/AccessControl.sol create mode 100644 src/dispatcherComponents/SweepTokens.sol create mode 100644 src/dispatcherComponents/Upgrade.sol create mode 100644 src/dispatcherComponents/ids.sol create mode 100644 src/testing/UpgradeTester.sol create mode 100644 test/dispatcherComponents/AccessControl.t.sol create mode 100644 test/dispatcherComponents/SweepTokens.t.sol create mode 100644 test/dispatcherComponents/Upgrade.t.sol create mode 100644 test/dispatcherComponents/utils/Dispatcher.sol create mode 100644 test/dispatcherComponents/utils/DispatcherTestBase.sol diff --git a/.gitmodules b/.gitmodules index 4c1b977..690924b 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,3 +1,6 @@ [submodule "lib/forge-std"] path = lib/forge-std - url = https://github.com/foundry-rs/forge-std \ No newline at end of file + url = https://github.com/foundry-rs/forge-std +[submodule "lib/openzeppelin-contracts"] + path = lib/openzeppelin-contracts + url = https://github.com/OpenZeppelin/openzeppelin-contracts diff --git a/foundry.toml b/foundry.toml index 2ab3aca..7016577 100644 --- a/foundry.toml +++ b/foundry.toml @@ -16,6 +16,7 @@ remappings = [ "forge-std/=lib/forge-std/src/", "wormhole-sdk/=src/", "IERC20/=src/interfaces/token/", + "@openzeppelin/=lib/openzeppelin-contracts/contracts/" ] verbosity = 3 diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts new file mode 160000 index 0000000..3291252 --- /dev/null +++ b/lib/openzeppelin-contracts @@ -0,0 +1 @@ +Subproject commit 3291252c866ad698f6a55ec660259e49a67eb3d0 diff --git a/src/TransferUtils.sol b/src/TransferUtils.sol new file mode 100644 index 0000000..a53efbf --- /dev/null +++ b/src/TransferUtils.sol @@ -0,0 +1,26 @@ + +// SPDX-License-Identifier: Apache 2 +pragma solidity ^0.8.19; + +import {SafeERC20} from "@openzeppelin/token/ERC20/utils/SafeERC20.sol"; +import {IERC20} from "@openzeppelin/token/ERC20/IERC20.sol"; + +/** + * Payment to the target failed. + */ +error PaymentFailure(address target); + +using SafeERC20 for IERC20; +function transferTokens(address token, address to, uint256 amount) { + if (token == address(0)) + _transferEth(to, amount); + else + IERC20(token).safeTransfer(to, amount); +} + +function _transferEth(address to, uint256 amount) { + if (amount == 0) return; + + (bool success, ) = to.call{value: amount}(new bytes(0)); + if (!success) revert PaymentFailure(to); +} \ No newline at end of file diff --git a/src/dispatcherComponents/AccessControl.sol b/src/dispatcherComponents/AccessControl.sol new file mode 100644 index 0000000..fd9cf48 --- /dev/null +++ b/src/dispatcherComponents/AccessControl.sol @@ -0,0 +1,247 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import "./ids.sol"; + +//rationale for different roles (owner, admin): +// * owner should be a mulit-sig / ultra cold wallet that is only activated in exceptional +// circumstances. +// * admin should also be either a cold wallet or Admin contract. In either case, +// the expectation is that multiple, slightly less trustworthy parties than the owner will +// have access to it, lowering trust assumptions and increasing attack surface. Admins +// perform rare but not exceptional operations. + +struct AccessControlState { + address owner; //puts owner address in eip1967 admin slot + address pendingOwner; + address[] admins; + mapping(address => uint256) isAdmin; +} + +// we use the designated eip1967 admin storage slot: +// keccak256("eip1967.proxy.admin") - 1 +bytes32 constant ACCESS_CONTROL_STORAGE_SLOT = + 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103; + +function accessControlState() pure returns (AccessControlState storage state) { + assembly ("memory-safe") { state.slot := ACCESS_CONTROL_STORAGE_SLOT } +} + +error NotAuthorized(); +error InvalidAccessControlCommand(uint8 command); +error InvalidAccessControlQuery(uint8 query); + +event OwnerUpdated(address oldAddress, address newAddress, uint256 timestamp); +event AdminsUpdated(address addr, bool isAdmin, uint256 timestamp); + +enum Role { + None, + Owner, + Admin +} + +function senderHasAuth() view returns (Role) { + Role role = senderRole(); + if (Role.None == role) + revert NotAuthorized(); + return role; +} + +function senderRole() view returns (Role) { + AccessControlState storage state = accessControlState(); + if (msg.sender == state.owner) //check highest privilege level first + return Role.Owner; + else if (state.isAdmin[msg.sender] != 0) + return Role.Admin; + else + return Role.None; +} + +abstract contract AccessControl { + using BytesParsing for bytes; + + // ---- construction ---- + + function _accessControlConstruction( + address owner, + address[] memory admins + ) internal { + accessControlState().owner = owner; + for (uint i = 0; i < admins.length; ++i) + _updateAdmins(admins[i], true); + } + + // ---- external ----- + + function transferOwnership(address newOwner) external { + AccessControlState storage state = accessControlState(); + if (msg.sender != state.owner) + revert NotAuthorized(); + + state.pendingOwner = newOwner; + } + + function cancelOwnershipTransfer() external { + AccessControlState storage state = accessControlState(); + if (state.owner != msg.sender) + revert NotAuthorized(); + + state.pendingOwner = address(0); + } + + function receiveOwnership() external { + _acquireOwnership(); + } + + // ---- internals ---- + + /** + * Dispatch an execute function. Execute functions almost always modify contract state. + */ + function dispatchExecAccessControl(bytes calldata data, uint256 offset, uint8 command) internal returns (bool, uint256) { + if (command == ACCESS_CONTROL_ID) + offset = _batchAccessControlCommands(data, offset); + else if (command == ACQUIRE_OWNERSHIP_ID) + _acquireOwnership(); + else return (false, offset); + + return (true, offset); + } + + /** + * Dispatch a query function. Query functions never modify contract state. + */ + function dispatchQueryAccessControl(bytes calldata data, uint256 offset, uint8 query) view internal returns (bool, bytes memory, uint256) { + bytes memory result; + if (query == ACCESS_CONTROL_QUERIES_ID) + (result, offset) = _batchAccessControlQueries(data, offset); + else return (false, new bytes(0), offset); + + return (true, result, offset); + } + + function _batchAccessControlCommands( + bytes calldata commands, + uint offset + ) internal returns (uint) { + AccessControlState storage state = accessControlState(); + bool isOwner = senderHasAuth() == Role.Owner; + + uint remainingCommands; + (remainingCommands, offset) = commands.asUint8CdUnchecked(offset); + for (uint i = 0; i < remainingCommands; ++i) { + uint8 command; + (command, offset) = commands.asUint8CdUnchecked(offset); + if (command == REVOKE_ADMIN_ID) { + address admin; + (admin, offset) = commands.asAddressCdUnchecked(offset); + _updateAdmins(admin, false); + } + else { + if (!isOwner) + revert NotAuthorized(); + + if (command == ADD_ADMIN_ID) { + address newAdmin; + (newAdmin, offset) = commands.asAddressCdUnchecked(offset); + + _updateAdmins(newAdmin, true); + } + else if (command == PROPOSE_OWNERSHIP_TRANSFER_ID) { + address newOwner; + (newOwner, offset) = commands.asAddressCdUnchecked(offset); + + state.pendingOwner = newOwner; + } + else if (command == RELINQUISH_OWNERSHIP_ID) { + _updateOwner(address(0)); + + //ownership relinquishment must be the last command in the batch + commands.checkLengthCd(offset); + } + else + revert InvalidAccessControlCommand(command); + } + } + return offset; + } + + function _batchAccessControlQueries( + bytes calldata queries, + uint offset + ) internal view returns (bytes memory, uint) { + AccessControlState storage state = accessControlState(); + bytes memory ret; + + uint remainingQueries; + (remainingQueries, offset) = queries.asUint8CdUnchecked(offset); + for (uint i = 0; i < remainingQueries; ++i) { + uint8 query; + (query, offset) = queries.asUint8CdUnchecked(offset); + + if (query == IS_ADMIN_ID) { + address admin; + (admin, offset) = queries.asAddressCdUnchecked(offset); + ret = abi.encodePacked(ret, state.isAdmin[admin] != 0); + } + else if (query == ADMINS_ID) { + ret = abi.encodePacked(ret, uint8(state.admins.length)); + for (uint j = 0; j < state.admins.length; ++j) + ret = abi.encodePacked(ret, state.admins[j]); + } + else { + address addr; + if (query == OWNER_ID) + addr = state.owner; + else if (query == PENDING_OWNER_ID) + addr = state.pendingOwner; + else + revert InvalidAccessControlQuery(query); + + ret = abi.encodePacked(ret, addr); + } + } + + return (ret, offset); + } + + function _acquireOwnership() internal { + AccessControlState storage state = accessControlState(); + if (state.pendingOwner != msg.sender) + revert NotAuthorized(); + + state.pendingOwner = address(0); + _updateOwner(msg.sender); + } + + // ---- private ---- + + function _updateOwner(address newOwner) private { + address oldAddress; + accessControlState().owner = newOwner; + emit OwnerUpdated(oldAddress, newOwner, block.timestamp); + } + + function _updateAdmins(address admin, bool authorization) private { unchecked { + AccessControlState storage state = accessControlState(); + if ((state.isAdmin[admin] != 0) == authorization) + return; + + if (authorization) { + state.admins.push(admin); + state.isAdmin[admin] = state.admins.length; + } + else { + uint256 rawIndex = state.isAdmin[admin]; + if (rawIndex != state.admins.length) + state.admins[rawIndex - 1] = state.admins[state.admins.length - 1]; + + state.isAdmin[admin] = 0; + state.admins.pop(); + } + + emit AdminsUpdated(admin, authorization, block.timestamp); + }} +} diff --git a/src/dispatcherComponents/SweepTokens.sol b/src/dispatcherComponents/SweepTokens.sol new file mode 100644 index 0000000..9f16da4 --- /dev/null +++ b/src/dispatcherComponents/SweepTokens.sol @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {transferTokens} from "../TransferUtils.sol"; +import {senderHasAuth} from "./AccessControl.sol"; +import "./ids.sol"; + +abstract contract SweepTokens { + using BytesParsing for bytes; + + /** + * Dispatch an execute function. Execute functions almost always modify contract state. + */ + function dispatchExecSweepTokens(bytes calldata data, uint256 offset, uint8 command) internal returns (bool, uint256) { + if (command == SWEEP_TOKENS_ID) + offset = _sweepTokens(data, offset); + else return (false, offset); + + return (true, offset); + } + + function _sweepTokens( + bytes calldata commands, + uint offset + ) internal returns (uint) { + senderHasAuth(); + + address token; + uint256 amount; + (token, offset) = commands.asAddressCdUnchecked(offset); + (amount, offset) = commands.asUint256CdUnchecked(offset); + + transferTokens(token, msg.sender, amount); + return offset; + } +} \ No newline at end of file diff --git a/src/dispatcherComponents/Upgrade.sol b/src/dispatcherComponents/Upgrade.sol new file mode 100644 index 0000000..6e99a28 --- /dev/null +++ b/src/dispatcherComponents/Upgrade.sol @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {ProxyBase} from "wormhole-sdk/proxy/ProxyBase.sol"; +import { + accessControlState, + AccessControlState, + NotAuthorized, + senderHasAuth, + Role +} from "./AccessControl.sol"; +import "./ids.sol"; + +error InvalidGovernanceCommand(uint8 command); +error InvalidGovernanceQuery(uint8 query); + +abstract contract Upgrade is ProxyBase { + using BytesParsing for bytes; + + /** + * Dispatch an execute function. Execute functions almost always modify contract state. + */ + function dispatchExecUpgrade(bytes calldata data, uint256 offset, uint8 command) internal returns (bool, uint256) { + if (command == UPGRADE_CONTRACT_ID) + offset = _upgradeContract(data, offset); + else return (false, offset); + + return (true, offset); + } + + /** + * Dispatch a query function. Query functions never modify contract state. + */ + function dispatchQueryUpgrade(bytes calldata, uint256 offset, uint8 query) view internal returns (bool, bytes memory, uint256) { + bytes memory result; + if (query == IMPLEMENTATION_ID) + result = abi.encodePacked(_getImplementation()); + else return (false, new bytes(0), offset); + + return (true, result, offset); + } + + function upgrade(address implementation, bytes calldata data) external { + if (senderHasAuth() != Role.Owner) + revert NotAuthorized(); + + _upgradeTo(implementation, data); + } + + function _upgradeContract( + bytes calldata commands, + uint offset + ) internal returns (uint) { + if (senderHasAuth() != Role.Owner) + revert NotAuthorized(); + + address newImplementation; + (newImplementation, offset) = commands.asAddressCdUnchecked(offset); + //contract upgrades must be the last command in the batch + commands.checkLengthCd(offset); + + _upgradeTo(newImplementation, new bytes(0)); + + return offset; + } +} diff --git a/src/dispatcherComponents/ids.sol b/src/dispatcherComponents/ids.sol new file mode 100644 index 0000000..2cfe31c --- /dev/null +++ b/src/dispatcherComponents/ids.sol @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +// ----------- Dispatcher Ids ----------- + +// Execute commands + +uint8 constant ACCESS_CONTROL_ID = 0x60; +uint8 constant ACQUIRE_OWNERSHIP_ID = 0x61; +uint8 constant UPGRADE_CONTRACT_ID = 0x62; +uint8 constant SWEEP_TOKENS_ID = 0x63; + +// Query commands + +uint8 constant ACCESS_CONTROL_QUERIES_ID = 0xe0; +uint8 constant IMPLEMENTATION_ID = 0xe1; + +// ----------- Access Control Ids ----------- + +// Execute commands + +//admin: +uint8 constant REVOKE_ADMIN_ID = 0x00; + +//owner only: +uint8 constant PROPOSE_OWNERSHIP_TRANSFER_ID = 0x10; +uint8 constant RELINQUISH_OWNERSHIP_ID = 0x11; +uint8 constant ADD_ADMIN_ID = 0x12; + +// Query commands + +uint8 constant OWNER_ID = 0x80; +uint8 constant PENDING_OWNER_ID = 0x81; +uint8 constant IS_ADMIN_ID = 0x82; +uint8 constant ADMINS_ID = 0x83; \ No newline at end of file diff --git a/src/testing/UpgradeTester.sol b/src/testing/UpgradeTester.sol new file mode 100644 index 0000000..9fd318f --- /dev/null +++ b/src/testing/UpgradeTester.sol @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.24; + +import {ProxyBase} from "wormhole-sdk/proxy/ProxyBase.sol"; + +contract UpgradeTester is ProxyBase { + event Constructed(bytes data); + event Upgraded(bytes data); + + function upgradeTo(address newImplementation, bytes calldata data) external { + _upgradeTo(newImplementation, data); + } + + function getImplementation() external view returns (address) { + return _getImplementation(); + } + + function _proxyConstructor(bytes calldata data) internal override { + emit Constructed(data); + } + + function _contractUpgrade(bytes calldata data) internal override { + emit Upgraded(data); + } +} diff --git a/test/dispatcherComponents/AccessControl.t.sol b/test/dispatcherComponents/AccessControl.t.sol new file mode 100644 index 0000000..435872f --- /dev/null +++ b/test/dispatcherComponents/AccessControl.t.sol @@ -0,0 +1,523 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import { NotAuthorized } from "wormhole-sdk/dispatcherComponents/AccessControl.sol"; +import { BytesParsing } from "wormhole-sdk/libraries/BytesParsing.sol"; +import { DispatcherTestBase } from "./utils/DispatcherTestBase.sol"; +import "wormhole-sdk/dispatcherComponents/ids.sol"; + +contract AcessControlTest is DispatcherTestBase { + using BytesParsing for bytes; + + function testCompleteOwnershipTransfer(address newOwner) public { + vm.assume(newOwner != address(this)); + uint8 commandCount = 1; + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + PROPOSE_OWNERSHIP_TRANSFER_ID, + newOwner + ) + ); + + commandCount = 2; + bytes memory getRes = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID + ) + ); + (address owner_, ) = getRes.asAddressUnchecked(0); + (address pendingOwner_, ) = getRes.asAddressUnchecked(20); + + assertEq(owner_, owner); + assertEq(pendingOwner_, newOwner); + + vm.prank(newOwner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACQUIRE_OWNERSHIP_ID + ) + ); + + getRes = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID + ) + ); + (owner_, ) = getRes.asAddressUnchecked(0); + (pendingOwner_, ) = getRes.asAddressUnchecked(20); + + assertEq(owner_, newOwner); + assertEq(pendingOwner_, address(0)); + } + + function testOwnershipTransfer_NotAuthorized() public { + uint8 commandCount = 1; + address newOwner = makeAddr("newOwner"); + + vm.expectRevert(NotAuthorized.selector); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + PROPOSE_OWNERSHIP_TRANSFER_ID, + newOwner + ) + ); + } + + function testOwnershipTransfer() public { + address newOwner = makeAddr("newOwner"); + uint8 commandCount = 1; + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + PROPOSE_OWNERSHIP_TRANSFER_ID, + newOwner + ) + ); + + commandCount = 2; + bytes memory getRes = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID + ) + ); + (address owner_, ) = getRes.asAddressUnchecked(0); + (address pendingOwner_, ) = getRes.asAddressUnchecked(20); + + assertEq(owner_, owner); + assertEq(pendingOwner_, newOwner); + } + + + + + + function testAcquireOwnership_NotAuthorized() public { + vm.expectRevert(NotAuthorized.selector); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACQUIRE_OWNERSHIP_ID + ) + ); + } + + function testAcquireOwnership() public { + address newOwner = makeAddr("newOwner"); + uint8 commandCount = 1; + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + PROPOSE_OWNERSHIP_TRANSFER_ID, + newOwner + ) + ); + + vm.prank(newOwner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACQUIRE_OWNERSHIP_ID + ) + ); + + bytes memory getRes = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID + ) + ); + (address owner_, ) = getRes.asAddressUnchecked(0); + (address pendingOwner_, ) = getRes.asAddressUnchecked(20); + + assertEq(owner_, newOwner); + assertEq(pendingOwner_, address(0)); + } + + function testExternalOwnershipTransfer_NotAuthorized() public { + address newOwner = makeAddr("newOwner"); + + vm.expectRevert(NotAuthorized.selector); + dispatcher.transferOwnership(newOwner); + } + + function testExternalOwnershipTransfer(address newOwner) public { + vm.prank(owner); + dispatcher.transferOwnership(newOwner); + + uint8 commandCount = 2; + bytes memory getRes = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID + ) + ); + (address owner_, ) = getRes.asAddressUnchecked(0); + (address pendingOwner_, ) = getRes.asAddressUnchecked(20); + + assertEq(owner_, owner); + assertEq(pendingOwner_, newOwner); + } + + function testExternalCancelOwnershipTransfer_NotAuthorized() public { + vm.expectRevert(NotAuthorized.selector); + dispatcher.cancelOwnershipTransfer(); + } + + function testExternalCancelOwnershipTransfer(address newOwner) public { + vm.prank(owner); + dispatcher.transferOwnership(newOwner); + + vm.prank(owner); + dispatcher.cancelOwnershipTransfer(); + + uint8 commandCount = 2; + bytes memory getRes = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID + ) + ); + (address owner_, ) = getRes.asAddressUnchecked(0); + (address pendingOwner_, ) = getRes.asAddressUnchecked(20); + + assertEq(owner_, owner); + assertEq(pendingOwner_, address(0)); + } + + function testExternalReceiveOwnership_NotAuthorized() public { + vm.expectRevert(NotAuthorized.selector); + dispatcher.receiveOwnership(); + } + + function testExternalReceiveOwnership(address newOwner) public { + vm.prank(owner); + dispatcher.transferOwnership(newOwner); + + vm.prank(newOwner); + dispatcher.receiveOwnership(); + + uint8 commandCount = 2; + bytes memory getRes = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID + ) + ); + (address owner_, ) = getRes.asAddressUnchecked(0); + (address pendingOwner_, ) = getRes.asAddressUnchecked(20); + + assertEq(owner_, newOwner); + assertEq(pendingOwner_, address(0)); + } + + function testBatchAfterAcquire(address newOwner, address newAdmin) public { + vm.assume(newOwner != address(0)); + vm.assume(newOwner != owner); + vm.assume(newAdmin != address(0)); + vm.assume(newAdmin != admin); + uint8 commandCount = 1; + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + PROPOSE_OWNERSHIP_TRANSFER_ID, + newOwner + ) + ); + + vm.prank(newOwner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACQUIRE_OWNERSHIP_ID, + ACCESS_CONTROL_ID, + commandCount, + ADD_ADMIN_ID, + newAdmin + ) + ); + + commandCount = 3; + bytes memory getRes = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID, + IS_ADMIN_ID, + newAdmin + ) + ); + uint offset = 0; + address owner_; + address pendingOwner_; + bool isAdmin; + (owner_, offset) = getRes.asAddressUnchecked(offset); + (pendingOwner_, offset) = getRes.asAddressUnchecked(offset); + (isAdmin, offset) = getRes.asBoolUnchecked(offset); + + assertEq(owner_, newOwner); + assertEq(pendingOwner_, address(0)); + assertEq(isAdmin, true); + } + + function testAddAdmin_NotAuthorized() public { + address newAdmin = makeAddr("newAdmin"); + uint8 commandCount = 1; + + vm.expectRevert(NotAuthorized.selector); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + ADD_ADMIN_ID, + newAdmin + ) + ); + + vm.expectRevert(NotAuthorized.selector); + vm.prank(admin); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + ADD_ADMIN_ID, + newAdmin + ) + ); + } + + function testAddAdmin(address newAdmin) public { + vm.assume(newAdmin != admin); + uint8 commandCount = 1; + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + ADD_ADMIN_ID, + newAdmin + ) + ); + + commandCount = 2; + bytes memory res = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + IS_ADMIN_ID, + newAdmin, + ADMINS_ID + ) + ); + + (bool isAdmin, ) = res.asBoolUnchecked(0); + (uint8 adminsCount, ) = res.asUint8Unchecked(1); + (address newAdmin_, ) = res.asAddressUnchecked(res.length - 20); + + assertEq(isAdmin, true); + assertEq(adminsCount, 2); + assertEq(newAdmin_, newAdmin); + } + + function testRevokeAdmin_NotAuthorized() public { + address newAdmin = makeAddr("newAdmin"); + uint8 commandCount = 1; + + vm.expectRevert(NotAuthorized.selector); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + REVOKE_ADMIN_ID, + newAdmin + ) + ); + } + + function testRevokeAdmin(address newAdmin) public { + vm.assume(newAdmin != admin); + uint8 commandCount = 1; + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + ADD_ADMIN_ID, + newAdmin + ) + ); + + commandCount = 1; + vm.prank(admin); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + REVOKE_ADMIN_ID, + newAdmin + ) + ); + + commandCount = 2; + bytes memory res = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + IS_ADMIN_ID, + newAdmin, + ADMINS_ID + ) + ); + + (bool isAdmin, ) = res.asBoolUnchecked(0); + (uint8 adminsCount, ) = res.asUint8Unchecked(1); + + assertEq(isAdmin, false); + assertEq(adminsCount, 1); + } + + function testRelinquishAdministration() public { + uint8 commandCount = 1; + + vm.prank(admin); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + REVOKE_ADMIN_ID, + admin + ) + ); + + bool isAdmin; + (isAdmin, ) = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + IS_ADMIN_ID, + admin + ) + ).asBoolUnchecked(0); + + assertEq(isAdmin, false); + } + + function testRelinquishOwnership_NotAuthorized() public { + uint8 commandCount = 1; + + vm.expectRevert(NotAuthorized.selector); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + RELINQUISH_OWNERSHIP_ID + ) + ); + } + + function testRelinquishOwnership_LengthMismatch() public { + uint8 commandCount = 2; + + vm.prank(owner); + vm.expectRevert( + abi.encodeWithSelector(BytesParsing.LengthMismatch.selector, 4, 3) + ); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + RELINQUISH_OWNERSHIP_ID, + ADD_ADMIN_ID + ) + ); + } + + + function testRelinquishOwnership() public { + uint8 commandCount = 1; + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + ACCESS_CONTROL_ID, + commandCount, + RELINQUISH_OWNERSHIP_ID + ) + ); + + (address owner_, ) = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID + ) + ).asAddressUnchecked(0); + + assertEq(owner_, address(0)); + } +} diff --git a/test/dispatcherComponents/SweepTokens.t.sol b/test/dispatcherComponents/SweepTokens.t.sol new file mode 100644 index 0000000..0fb1d96 --- /dev/null +++ b/test/dispatcherComponents/SweepTokens.t.sol @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {SWEEP_TOKENS_ID} from "wormhole-sdk/dispatcherComponents/ids.sol"; +import {DispatcherTestBase} from "./utils/DispatcherTestBase.sol"; +import {ERC20Mock} from "wormhole-sdk/testing/ERC20Mock.sol"; + +contract SweepTokensTest is DispatcherTestBase { + ERC20Mock token; + + function _setUp1() internal override { + token = new ERC20Mock("FakeToken", "FT"); + } + + function testSweepTokens_erc20() public { + uint tokenAmount = 1e6; + deal(address(token), address(dispatcher), tokenAmount); + + assertEq(token.balanceOf(owner), 0); + assertEq(token.balanceOf(address(dispatcher)), tokenAmount); + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + SWEEP_TOKENS_ID, address(token), tokenAmount + ) + ); + + assertEq(token.balanceOf(address(dispatcher)), 0); + assertEq(token.balanceOf(owner), tokenAmount); + } + + function testSweepTokens_eth() public { + uint ethAmount = 1 ether; + vm.deal(address(dispatcher), ethAmount); + uint ownerEthBalance = address(owner).balance; + assertEq(address(dispatcher).balance, ethAmount); + + vm.prank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + SWEEP_TOKENS_ID, address(0), ethAmount + ) + ); + + assertEq(address(dispatcher).balance, 0); + assertEq(address(owner).balance, ownerEthBalance + ethAmount); + } +} diff --git a/test/dispatcherComponents/Upgrade.t.sol b/test/dispatcherComponents/Upgrade.t.sol new file mode 100644 index 0000000..b3c50de --- /dev/null +++ b/test/dispatcherComponents/Upgrade.t.sol @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {NotAuthorized} from "wormhole-sdk/dispatcherComponents/AccessControl.sol"; +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {UpgradeTester} from "wormhole-sdk/testing/UpgradeTester.sol"; +import {IdempotentUpgrade} from "wormhole-sdk/proxy/ProxyBase.sol"; +import {DispatcherTestBase} from "./utils/DispatcherTestBase.sol"; +import "wormhole-sdk/dispatcherComponents/ids.sol"; + +contract UpgradeTest is DispatcherTestBase { + using BytesParsing for bytes; + + function testContractUpgrade_NotAuthorized() public { + address fakeAddress = makeAddr("fakeAddress"); + + vm.expectRevert(NotAuthorized.selector); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + UPGRADE_CONTRACT_ID, + address(fakeAddress) + ) + ); + + vm.prank(admin); + vm.expectRevert(NotAuthorized.selector); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + UPGRADE_CONTRACT_ID, + address(fakeAddress) + ) + ); + } + + function testContractUpgrade_IdempotentUpgrade() public { + UpgradeTester upgradeTester = new UpgradeTester(); + + vm.startPrank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + UPGRADE_CONTRACT_ID, + address(upgradeTester) + ) + ); + + vm.expectRevert(IdempotentUpgrade.selector); + UpgradeTester(address(dispatcher)).upgradeTo(address(upgradeTester), new bytes(0)); + } + + function testContractUpgrade() public { + UpgradeTester upgradeTester = new UpgradeTester(); + + bytes memory response = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + IMPLEMENTATION_ID + ) + ); + assertEq(response.length, 20); + (address implementation,) = response.asAddressUnchecked(0); + + vm.startPrank(owner); + invokeDispatcher( + abi.encodePacked( + dispatcher.exec768.selector, + UPGRADE_CONTRACT_ID, + address(upgradeTester) + ) + ); + + UpgradeTester(address(dispatcher)).upgradeTo(implementation, new bytes(0)); + + response = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + IMPLEMENTATION_ID + ) + ); + assertEq(response.length, 20); + (address restoredImplementation,) = response.asAddressUnchecked(0); + assertEq(restoredImplementation, implementation); + } + + function testExternalContractUpgrade_NotAuthorized() public { + address fakeAddress = makeAddr("fakeAddress"); + + vm.expectRevert(NotAuthorized.selector); + dispatcher.upgrade(address(fakeAddress), new bytes(0)); + + vm.prank(admin); + vm.expectRevert(NotAuthorized.selector); + dispatcher.upgrade(address(fakeAddress), new bytes(0)); + } + + function testExternalContractUpgrade_IdempotentUpgrade() public { + UpgradeTester upgradeTester = new UpgradeTester(); + + vm.startPrank(owner); + dispatcher.upgrade(address(upgradeTester), new bytes(0)); + + vm.expectRevert(IdempotentUpgrade.selector); + UpgradeTester(address(dispatcher)).upgradeTo(address(upgradeTester), new bytes(0)); + } + + function testExternalContractUpgrade() public { + UpgradeTester upgradeTester = new UpgradeTester(); + + bytes memory response = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + IMPLEMENTATION_ID + ) + ); + assertEq(response.length, 20); + (address implementation,) = response.asAddressUnchecked(0); + + vm.startPrank(owner); + dispatcher.upgrade(address(upgradeTester), new bytes(0)); + + UpgradeTester(address(dispatcher)).upgradeTo(implementation, new bytes(0)); + + response = invokeStaticDispatcher( + abi.encodePacked( + dispatcher.get1959.selector, + IMPLEMENTATION_ID + ) + ); + assertEq(response.length, 20); + (address restoredImplementation,) = response.asAddressUnchecked(0); + assertEq(restoredImplementation, implementation); + } +} diff --git a/test/dispatcherComponents/utils/Dispatcher.sol b/test/dispatcherComponents/utils/Dispatcher.sol new file mode 100644 index 0000000..8224877 --- /dev/null +++ b/test/dispatcherComponents/utils/Dispatcher.sol @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {AccessControl} from "wormhole-sdk/dispatcherComponents/AccessControl.sol"; +import {SweepTokens} from "wormhole-sdk/dispatcherComponents/SweepTokens.sol"; +import {Upgrade} from "wormhole-sdk/dispatcherComponents/Upgrade.sol"; +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {RawDispatcher} from "wormhole-sdk/RawDispatcher.sol"; + +contract Dispatcher is RawDispatcher, AccessControl, SweepTokens, Upgrade { + using BytesParsing for bytes; + + function _proxyConstructor(bytes calldata args) internal override { + uint offset = 0; + + address owner; + (owner, offset) = args.asAddressCdUnchecked(offset); + + uint8 adminCount; + (adminCount, offset) = args.asUint8CdUnchecked(offset); + address[] memory admins = new address[](adminCount); + for (uint i = 0; i < adminCount; ++i) { + (admins[i], offset) = args.asAddressCdUnchecked(offset); + } + args.checkLengthCd(offset); + + _accessControlConstruction(owner, admins); + } + + function _exec(bytes calldata data) internal override returns (bytes memory) { unchecked { + uint offset = 0; + + while (offset < data.length) { + uint8 command; + (command, offset) = data.asUint8CdUnchecked(offset); + + bool dispatched; + (dispatched, offset) = dispatchExecAccessControl(data, offset, command); + if (!dispatched) + (dispatched, offset) = dispatchExecUpgrade(data, offset, command); + if (!dispatched) + (dispatched, offset) = dispatchExecSweepTokens(data, offset, command); + } + + data.checkLengthCd(offset); + return new bytes(0); + }} + + function _get(bytes calldata data) internal view override returns (bytes memory) { unchecked { + bytes memory ret; + uint offset = 0; + + while (offset < data.length) { + uint8 query; + (query, offset) = data.asUint8CdUnchecked(offset); + + bytes memory result; + bool dispatched; + (dispatched, result, offset) = dispatchQueryAccessControl(data, offset, query); + if (!dispatched) + (dispatched, result, offset) = dispatchQueryUpgrade(data, offset, query); + + ret = abi.encodePacked(ret, result); + } + data.checkLengthCd(offset); + return ret; + }} +} \ No newline at end of file diff --git a/test/dispatcherComponents/utils/DispatcherTestBase.sol b/test/dispatcherComponents/utils/DispatcherTestBase.sol new file mode 100644 index 0000000..59f067a --- /dev/null +++ b/test/dispatcherComponents/utils/DispatcherTestBase.sol @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {Proxy} from "wormhole-sdk/proxy/Proxy.sol"; +import {reRevert} from "wormhole-sdk/Utils.sol"; +import {Dispatcher} from "./Dispatcher.sol"; +import "forge-std/Test.sol"; + +contract DispatcherTestBase is Test { + using BytesParsing for bytes; + + address immutable owner; + address immutable admin; + + address dispatcherImplementation; + Dispatcher dispatcher; + + constructor() { + owner = makeAddr("owner"); + admin = makeAddr("admin"); + } + + function _setUp1() internal virtual { } + + function setUp() public { + uint8 adminCount = 1; + + dispatcherImplementation = address(new Dispatcher()); + dispatcher = Dispatcher(address(new Proxy( + dispatcherImplementation, + abi.encodePacked( + owner, + adminCount, + admin + ) + ))); + + _setUp1(); + } + + function invokeStaticDispatcher(bytes memory encoded) view internal returns (bytes memory data) { + (bool success, bytes memory result) = address(dispatcher).staticcall(encoded); + if (!success) { + reRevert(result); + } + (uint length,) = result.asUint256Unchecked(32); + (data,) = result.sliceUnchecked(64, length); + } + + function invokeDispatcher(bytes memory encoded) internal returns (bytes memory data) { + (bool success, bytes memory result) = address(dispatcher).call(encoded); + if (!success) { + reRevert(result); + } + (uint length,) = result.asUint256Unchecked(32); + (data,) = result.sliceUnchecked(64, length); + } + + function invokeDispatcher(bytes memory encoded, uint value) internal returns (bytes memory data) { + (bool success, bytes memory result) = address(dispatcher).call{value: value}(encoded); + if (!success) { + reRevert(result); + } + (uint length,) = result.asUint256Unchecked(32); + (data,) = result.sliceUnchecked(64, length); + } +} \ No newline at end of file From df355aec1005dabca4ebad32dd534c4e49b9d782 Mon Sep 17 00:00:00 2001 From: nonergodic Date: Mon, 18 Nov 2024 16:29:46 -0800 Subject: [PATCH 2/9] address issues raised in PR --- README.md | 4 +- foundry.toml | 2 +- src/TransferUtils.sol | 26 ------- src/Utils.sol | 27 +------- .../dispatcher}/AccessControl.sol | 69 ++++++++++++------- .../ids.sol => components/dispatcher/Ids.sol} | 0 src/components/dispatcher/SweepTokens.sol | 37 ++++++++++ src/components/dispatcher/Upgrade.sol | 57 +++++++++++++++ src/constants/Common.sol | 14 ++-- src/dispatcherComponents/SweepTokens.sol | 38 ---------- src/dispatcherComponents/Upgrade.sol | 68 ------------------ src/libraries/BytesParsing.sol | 2 +- src/libraries/SafeERC20.sol | 67 ++++++++++++++++++ src/utils/Revert.sol | 11 +++ src/utils/Transfer.sol | 23 +++++++ src/utils/UniversalAddress.sol | 17 +++++ .../dispatcher}/AccessControl.t.sol | 20 ++++-- .../dispatcher}/SweepTokens.t.sol | 5 +- .../dispatcher}/Upgrade.t.sol | 15 ++-- .../dispatcher}/utils/Dispatcher.sol | 8 +-- .../dispatcher}/utils/DispatcherTestBase.sol | 9 +-- 21 files changed, 309 insertions(+), 210 deletions(-) delete mode 100644 src/TransferUtils.sol rename src/{dispatcherComponents => components/dispatcher}/AccessControl.sol (85%) rename src/{dispatcherComponents/ids.sol => components/dispatcher/Ids.sol} (100%) create mode 100644 src/components/dispatcher/SweepTokens.sol create mode 100644 src/components/dispatcher/Upgrade.sol delete mode 100644 src/dispatcherComponents/SweepTokens.sol delete mode 100644 src/dispatcherComponents/Upgrade.sol create mode 100644 src/libraries/SafeERC20.sol create mode 100644 src/utils/Revert.sol create mode 100644 src/utils/Transfer.sol create mode 100644 src/utils/UniversalAddress.sol rename test/{dispatcherComponents => components/dispatcher}/AccessControl.t.sol (95%) rename test/{dispatcherComponents => components/dispatcher}/SweepTokens.t.sol (85%) rename test/{dispatcherComponents => components/dispatcher}/Upgrade.t.sol (87%) rename test/{dispatcherComponents => components/dispatcher}/utils/Dispatcher.sol (86%) rename test/{dispatcherComponents => components/dispatcher}/utils/DispatcherTestBase.sol (91%) diff --git a/README.md b/README.md index e65e6bd..b4166c6 100644 --- a/README.md +++ b/README.md @@ -36,9 +36,9 @@ For example, if you are using a solc version newer than `0.8.19` and are plannin It is strongly recommended that you run the forge test suite of this SDK with your own compiler version to catch potential errors that stem from differences in compiler versions early. Yes, strictly speaking the Solidity version pragma should prevent these issues, but better to be safe than sorry, especially given that some components make extensive use of inline assembly. -**IERC20 Remapping** +**IERC20 and SafeERC20 Remapping** -This SDK comes with its own IERC20 interface. Given that projects tend to combine different SDKs, there's often this annoying issue of clashes of IERC20 interfaces, even though the are effectively the same. We handle this issue by importing `IERC20/IERC20.sol` which allows remapping the `IERC20/` prefix to whatever directory contains `IERC20.sol` in your project, thus providing an override mechanism that should allow dealing with this problem seamlessly until forge allows remapping of individual files. +This SDK comes with its own IERC20 interface and SafeERC20 implementation. Given that projects tend to combine different SDKs, there's often this annoying issue of clashes of IERC20 interfaces, even though the are effectively the same. We handle this issue by importing `IERC20/IERC20.sol` which allows remapping the `IERC20/` prefix to whatever directory contains `IERC20.sol` in your project, thus providing an override mechanism that should allow dealing with this problem seamlessly until forge allows remapping of individual files. The same approach is used for SafeERC20. ## Components diff --git a/foundry.toml b/foundry.toml index 7016577..f553a94 100644 --- a/foundry.toml +++ b/foundry.toml @@ -16,7 +16,7 @@ remappings = [ "forge-std/=lib/forge-std/src/", "wormhole-sdk/=src/", "IERC20/=src/interfaces/token/", - "@openzeppelin/=lib/openzeppelin-contracts/contracts/" + "SafeERC20/=src/libraries/", ] verbosity = 3 diff --git a/src/TransferUtils.sol b/src/TransferUtils.sol deleted file mode 100644 index a53efbf..0000000 --- a/src/TransferUtils.sol +++ /dev/null @@ -1,26 +0,0 @@ - -// SPDX-License-Identifier: Apache 2 -pragma solidity ^0.8.19; - -import {SafeERC20} from "@openzeppelin/token/ERC20/utils/SafeERC20.sol"; -import {IERC20} from "@openzeppelin/token/ERC20/IERC20.sol"; - -/** - * Payment to the target failed. - */ -error PaymentFailure(address target); - -using SafeERC20 for IERC20; -function transferTokens(address token, address to, uint256 amount) { - if (token == address(0)) - _transferEth(to, amount); - else - IERC20(token).safeTransfer(to, amount); -} - -function _transferEth(address to, uint256 amount) { - if (amount == 0) return; - - (bool success, ) = to.call{value: amount}(new bytes(0)); - if (!success) revert PaymentFailure(to); -} \ No newline at end of file diff --git a/src/Utils.sol b/src/Utils.sol index 63949f8..4b39b85 100644 --- a/src/Utils.sol +++ b/src/Utils.sol @@ -2,27 +2,6 @@ // SPDX-License-Identifier: Apache 2 pragma solidity ^0.8.19; -error NotAnEvmAddress(bytes32); - -function toUniversalAddress(address addr) pure returns (bytes32 universalAddr) { - universalAddr = bytes32(uint256(uint160(addr))); -} - -function fromUniversalAddress(bytes32 universalAddr) pure returns (address addr) { - if (bytes12(universalAddr) != 0) - revert NotAnEvmAddress(universalAddr); - - assembly ("memory-safe") { - addr := universalAddr - } -} - -/** - * Reverts with a given buffer data. - * Meant to be used to easily bubble up errors from low level calls when they fail. - */ -function reRevert(bytes memory err) pure { - assembly ("memory-safe") { - revert(add(err, 32), mload(err)) - } -} +import {tokenOrNativeTransfer} from "wormhole-sdk/utils/Transfer.sol"; +import {reRevert} from "wormhole-sdk/utils/Revert.sol"; +import {toUniversalAddress, fromUniversalAddress} from "wormhole-sdk/utils/UniversalAddress.sol"; diff --git a/src/dispatcherComponents/AccessControl.sol b/src/components/dispatcher/AccessControl.sol similarity index 85% rename from src/dispatcherComponents/AccessControl.sol rename to src/components/dispatcher/AccessControl.sol index fd9cf48..2d617c5 100644 --- a/src/dispatcherComponents/AccessControl.sol +++ b/src/components/dispatcher/AccessControl.sol @@ -3,7 +3,19 @@ pragma solidity ^0.8.4; import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; -import "./ids.sol"; +import { + ACCESS_CONTROL_ID, + ACCESS_CONTROL_QUERIES_ID, + OWNER_ID, + PENDING_OWNER_ID, + IS_ADMIN_ID, + ADMINS_ID, + REVOKE_ADMIN_ID, + ADD_ADMIN_ID, + PROPOSE_OWNERSHIP_TRANSFER_ID, + ACQUIRE_OWNERSHIP_ID, + RELINQUISH_OWNERSHIP_ID +} from "wormhole-sdk/components/dispatcher/Ids.sol"; //rationale for different roles (owner, admin): // * owner should be a mulit-sig / ultra cold wallet that is only activated in exceptional @@ -42,10 +54,15 @@ enum Role { Admin } -function senderHasAuth() view returns (Role) { - Role role = senderRole(); - if (Role.None == role) +function failAuthIf(bool condition) pure { + if (condition) revert NotAuthorized(); +} + +function senderAtLeastAdmin() view returns (Role) { + Role role = senderRole(); + failAuthIf(role == Role.None); + return role; } @@ -53,10 +70,8 @@ function senderRole() view returns (Role) { AccessControlState storage state = accessControlState(); if (msg.sender == state.owner) //check highest privilege level first return Role.Owner; - else if (state.isAdmin[msg.sender] != 0) - return Role.Admin; - else - return Role.None; + + return state.isAdmin[msg.sender] != 0 ? Role.Admin : Role.None; } abstract contract AccessControl { @@ -77,16 +92,14 @@ abstract contract AccessControl { function transferOwnership(address newOwner) external { AccessControlState storage state = accessControlState(); - if (msg.sender != state.owner) - revert NotAuthorized(); + failAuthIf(msg.sender != state.owner); state.pendingOwner = newOwner; } function cancelOwnershipTransfer() external { AccessControlState storage state = accessControlState(); - if (state.owner != msg.sender) - revert NotAuthorized(); + failAuthIf(msg.sender != state.owner); state.pendingOwner = address(0); } @@ -97,27 +110,31 @@ abstract contract AccessControl { // ---- internals ---- - /** - * Dispatch an execute function. Execute functions almost always modify contract state. - */ - function dispatchExecAccessControl(bytes calldata data, uint256 offset, uint8 command) internal returns (bool, uint256) { + function dispatchExecAccessControl( + bytes calldata data, + uint offset, + uint8 command + ) internal returns (bool, uint) { if (command == ACCESS_CONTROL_ID) offset = _batchAccessControlCommands(data, offset); else if (command == ACQUIRE_OWNERSHIP_ID) _acquireOwnership(); - else return (false, offset); + else + return (false, offset); return (true, offset); } - /** - * Dispatch a query function. Query functions never modify contract state. - */ - function dispatchQueryAccessControl(bytes calldata data, uint256 offset, uint8 query) view internal returns (bool, bytes memory, uint256) { + function dispatchQueryAccessControl( + bytes calldata data, + uint offset, + uint8 query + ) view internal returns (bool, bytes memory, uint) { bytes memory result; if (query == ACCESS_CONTROL_QUERIES_ID) (result, offset) = _batchAccessControlQueries(data, offset); - else return (false, new bytes(0), offset); + else + return (false, new bytes(0), offset); return (true, result, offset); } @@ -127,7 +144,7 @@ abstract contract AccessControl { uint offset ) internal returns (uint) { AccessControlState storage state = accessControlState(); - bool isOwner = senderHasAuth() == Role.Owner; + bool isOwner = senderAtLeastAdmin() == Role.Owner; uint remainingCommands; (remainingCommands, offset) = commands.asUint8CdUnchecked(offset); @@ -180,12 +197,12 @@ abstract contract AccessControl { for (uint i = 0; i < remainingQueries; ++i) { uint8 query; (query, offset) = queries.asUint8CdUnchecked(offset); - + if (query == IS_ADMIN_ID) { address admin; (admin, offset) = queries.asAddressCdUnchecked(offset); ret = abi.encodePacked(ret, state.isAdmin[admin] != 0); - } + } else if (query == ADMINS_ID) { ret = abi.encodePacked(ret, uint8(state.admins.length)); for (uint j = 0; j < state.admins.length; ++j) @@ -209,7 +226,7 @@ abstract contract AccessControl { function _acquireOwnership() internal { AccessControlState storage state = accessControlState(); - if (state.pendingOwner != msg.sender) + if (msg.sender !=state.pendingOwner) revert NotAuthorized(); state.pendingOwner = address(0); diff --git a/src/dispatcherComponents/ids.sol b/src/components/dispatcher/Ids.sol similarity index 100% rename from src/dispatcherComponents/ids.sol rename to src/components/dispatcher/Ids.sol diff --git a/src/components/dispatcher/SweepTokens.sol b/src/components/dispatcher/SweepTokens.sol new file mode 100644 index 0000000..780cae5 --- /dev/null +++ b/src/components/dispatcher/SweepTokens.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {tokenOrNativeTransfer} from "wormhole-sdk/utils/Transfer.sol"; +import {senderAtLeastAdmin} from "wormhole-sdk/components/dispatcher/AccessControl.sol"; +import {SWEEP_TOKENS_ID} from "wormhole-sdk/components/dispatcher/Ids.sol"; + +abstract contract SweepTokens { + using BytesParsing for bytes; + + function dispatchExecSweepTokens( + bytes calldata data, + uint offset, + uint8 command + ) internal returns (bool, uint) { + return command == SWEEP_TOKENS_ID + ? (true, _sweepTokens(data, offset)) + : (false, offset); + } + + function _sweepTokens( + bytes calldata commands, + uint offset + ) internal returns (uint) { + senderAtLeastAdmin(); + + address token; + uint256 amount; + (token, offset) = commands.asAddressCdUnchecked(offset); + (amount, offset) = commands.asUint256CdUnchecked(offset); + + tokenOrNativeTransfer(token, msg.sender, amount); + return offset; + } +} \ No newline at end of file diff --git a/src/components/dispatcher/Upgrade.sol b/src/components/dispatcher/Upgrade.sol new file mode 100644 index 0000000..bba180c --- /dev/null +++ b/src/components/dispatcher/Upgrade.sol @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: Apache 2 + +pragma solidity ^0.8.4; + +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {ProxyBase} from "wormhole-sdk/proxy/ProxyBase.sol"; +import {Role, senderRole, failAuthIf} from "wormhole-sdk/components/dispatcher/AccessControl.sol"; +import {UPGRADE_CONTRACT_ID, IMPLEMENTATION_ID} from "wormhole-sdk/components/dispatcher/Ids.sol"; + +error InvalidGovernanceCommand(uint8 command); +error InvalidGovernanceQuery(uint8 query); + +abstract contract Upgrade is ProxyBase { + using BytesParsing for bytes; + + function dispatchExecUpgrade( + bytes calldata data, + uint offset, + uint8 command + ) internal returns (bool, uint) { + return (command == UPGRADE_CONTRACT_ID) + ? (true, _upgradeContract(data, offset)) + : (false, offset); + } + + function dispatchQueryUpgrade( + bytes calldata, + uint offset, + uint8 query + ) view internal returns (bool, bytes memory, uint) { + return query == IMPLEMENTATION_ID + ? (true, abi.encodePacked(_getImplementation()), offset) + : (false, new bytes(0), offset); + } + + function upgrade(address implementation, bytes calldata data) external { + failAuthIf(senderRole() != Role.Owner); + + _upgradeTo(implementation, data); + } + + function _upgradeContract( + bytes calldata commands, + uint offset + ) internal returns (uint) { + failAuthIf(senderRole() != Role.Owner); + + address newImplementation; + (newImplementation, offset) = commands.asAddressCdUnchecked(offset); + //contract upgrades must be the last command in the batch + commands.checkLengthCd(offset); + + _upgradeTo(newImplementation, new bytes(0)); + + return offset; + } +} diff --git a/src/constants/Common.sol b/src/constants/Common.sol index 27f19d9..f3ac5f5 100644 --- a/src/constants/Common.sol +++ b/src/constants/Common.sol @@ -1,11 +1,17 @@ // SPDX-License-Identifier: Apache 2 pragma solidity ^0.8.4; -uint256 constant FREE_MEMORY_PTR = 0x40; +// ┌──────────────────────────────────────────────────────────────────────────────┐ +// │ NOTE: We can't define e.g. WORD_SIZE_MINUS_ONE via WORD_SIZE - 1 because │ +// │ of solc restrictions on what constants can be used in inline assembly. │ +// └──────────────────────────────────────────────────────────────────────────────┘ + uint256 constant WORD_SIZE = 32; -//we can't define _WORD_SIZE_MINUS_ONE via _WORD_SIZE - 1 because of solc restrictions -// what constants can be used in inline assembly uint256 constant WORD_SIZE_MINUS_ONE = 31; //=0x1f=0b00011111 - //see section "prefer `< MAX + 1` over `<= MAX` for const comparison" in docs/Optimization.md uint256 constant WORD_SIZE_PLUS_ONE = 33; + +uint256 constant SCRATCH_SPACE_PTR = 0x00; +uint256 constant SCRATCH_SPACE_SIZE = 64; + +uint256 constant FREE_MEMORY_PTR = 0x40; \ No newline at end of file diff --git a/src/dispatcherComponents/SweepTokens.sol b/src/dispatcherComponents/SweepTokens.sol deleted file mode 100644 index 9f16da4..0000000 --- a/src/dispatcherComponents/SweepTokens.sol +++ /dev/null @@ -1,38 +0,0 @@ -// SPDX-License-Identifier: Apache 2 - -pragma solidity ^0.8.4; - -import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; -import {transferTokens} from "../TransferUtils.sol"; -import {senderHasAuth} from "./AccessControl.sol"; -import "./ids.sol"; - -abstract contract SweepTokens { - using BytesParsing for bytes; - - /** - * Dispatch an execute function. Execute functions almost always modify contract state. - */ - function dispatchExecSweepTokens(bytes calldata data, uint256 offset, uint8 command) internal returns (bool, uint256) { - if (command == SWEEP_TOKENS_ID) - offset = _sweepTokens(data, offset); - else return (false, offset); - - return (true, offset); - } - - function _sweepTokens( - bytes calldata commands, - uint offset - ) internal returns (uint) { - senderHasAuth(); - - address token; - uint256 amount; - (token, offset) = commands.asAddressCdUnchecked(offset); - (amount, offset) = commands.asUint256CdUnchecked(offset); - - transferTokens(token, msg.sender, amount); - return offset; - } -} \ No newline at end of file diff --git a/src/dispatcherComponents/Upgrade.sol b/src/dispatcherComponents/Upgrade.sol deleted file mode 100644 index 6e99a28..0000000 --- a/src/dispatcherComponents/Upgrade.sol +++ /dev/null @@ -1,68 +0,0 @@ -// SPDX-License-Identifier: Apache 2 - -pragma solidity ^0.8.4; - -import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; -import {ProxyBase} from "wormhole-sdk/proxy/ProxyBase.sol"; -import { - accessControlState, - AccessControlState, - NotAuthorized, - senderHasAuth, - Role -} from "./AccessControl.sol"; -import "./ids.sol"; - -error InvalidGovernanceCommand(uint8 command); -error InvalidGovernanceQuery(uint8 query); - -abstract contract Upgrade is ProxyBase { - using BytesParsing for bytes; - - /** - * Dispatch an execute function. Execute functions almost always modify contract state. - */ - function dispatchExecUpgrade(bytes calldata data, uint256 offset, uint8 command) internal returns (bool, uint256) { - if (command == UPGRADE_CONTRACT_ID) - offset = _upgradeContract(data, offset); - else return (false, offset); - - return (true, offset); - } - - /** - * Dispatch a query function. Query functions never modify contract state. - */ - function dispatchQueryUpgrade(bytes calldata, uint256 offset, uint8 query) view internal returns (bool, bytes memory, uint256) { - bytes memory result; - if (query == IMPLEMENTATION_ID) - result = abi.encodePacked(_getImplementation()); - else return (false, new bytes(0), offset); - - return (true, result, offset); - } - - function upgrade(address implementation, bytes calldata data) external { - if (senderHasAuth() != Role.Owner) - revert NotAuthorized(); - - _upgradeTo(implementation, data); - } - - function _upgradeContract( - bytes calldata commands, - uint offset - ) internal returns (uint) { - if (senderHasAuth() != Role.Owner) - revert NotAuthorized(); - - address newImplementation; - (newImplementation, offset) = commands.asAddressCdUnchecked(offset); - //contract upgrades must be the last command in the batch - commands.checkLengthCd(offset); - - _upgradeTo(newImplementation, new bytes(0)); - - return offset; - } -} diff --git a/src/libraries/BytesParsing.sol b/src/libraries/BytesParsing.sol index 86a0aac..9f886af 100644 --- a/src/libraries/BytesParsing.sol +++ b/src/libraries/BytesParsing.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache 2 pragma solidity ^0.8.4; -import "../constants/Common.sol"; +import "wormhole-sdk/constants/Common.sol"; //This file appears comically large, but all unused functions are removed by the compiler. library BytesParsing { diff --git a/src/libraries/SafeERC20.sol b/src/libraries/SafeERC20.sol new file mode 100644 index 0000000..dfa7e90 --- /dev/null +++ b/src/libraries/SafeERC20.sol @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache 2 +pragma solidity ^0.8.4; + +import {IERC20} from "IERC20/IERC20.sol"; +import {WORD_SIZE, SCRATCH_SPACE_PTR} from "wormhole-sdk/constants/Common.sol"; + +//Like OpenZeppelin's SafeERC20.sol, but slimmed down and more gas efficient. +// +//The main difference to OZ's implementation (besides the missing functions) is that we skip the +// EXTCODESIZE check that OZ does upon successful calls to ensure that an actual contract was +// called. The rationale for omitting this check is that ultimately the contract using the token +// has to verify that it "makes sense" for its use case regardless. Otherwise, a random token, or +// even just a contract that always returns true, could be passed, which makes this check +// superfluous in the final analysis. +// +//We also save on code size by not duplicating the assembly code in two separate functions. +// Otoh, we simply swallow revert reasons of failing token operations instead of bubbling them up. +// This is less clean and makes debugging harder, but is likely still a worthwhile trade-off +// given the cost in gas and code size. +library SafeERC20 { + error SafeERC20FailedOperation(address token); + + function safeTransfer(IERC20 token, address to, uint256 value) internal { + _revertOnFailure(token, abi.encodeCall(token.transfer, (to, value))); + } + + function safeTransferFrom(IERC20 token, address from, address to, uint256 value) internal { + _revertOnFailure(token, abi.encodeCall(token.transferFrom, (from, to, value))); + } + + function forceApprove(IERC20 token, address spender, uint256 value) internal { + bytes memory approveCall = abi.encodeCall(token.approve, (spender, value)); + + if (!_callWithOptionalReturnCheck(token, approveCall)) { + _revertOnFailure(token, abi.encodeCall(token.approve, (spender, 0))); + _revertOnFailure(token, approveCall); + } + } + + function _callWithOptionalReturnCheck( + IERC20 token, + bytes memory encodedCall + ) private returns (bool success) { + /// @solidity memory-safe-assembly + assembly { + mstore(SCRATCH_SPACE_PTR, 0) + success := call( //see https://www.evm.codes/?fork=cancun#f1 + gas(), //gas + token, //callee + 0, //value + add(encodedCall, WORD_SIZE), //input ptr + mload(encodedCall), //input size + SCRATCH_SPACE_PTR, //output ptr + WORD_SIZE //output size + ) + //calls to addresses without code are always successful + if success { + success := or(iszero(returndatasize()), mload(SCRATCH_SPACE_PTR)) + } + } + } + + function _revertOnFailure(IERC20 token, bytes memory encodedCall) private { + if (!_callWithOptionalReturnCheck(token, encodedCall)) + revert SafeERC20FailedOperation(address(token)); + } +} diff --git a/src/utils/Revert.sol b/src/utils/Revert.sol new file mode 100644 index 0000000..c5c2928 --- /dev/null +++ b/src/utils/Revert.sol @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache 2 +pragma solidity ^0.8.19; + +import {WORD_SIZE} from "wormhole-sdk/constants/Common.sol"; + +//bubble up errors from low level calls +function reRevert(bytes memory err) pure { + assembly ("memory-safe") { + revert(add(err, WORD_SIZE), mload(err)) + } +} diff --git a/src/utils/Transfer.sol b/src/utils/Transfer.sol new file mode 100644 index 0000000..c1774b2 --- /dev/null +++ b/src/utils/Transfer.sol @@ -0,0 +1,23 @@ + +// SPDX-License-Identifier: Apache 2 +pragma solidity ^0.8.19; + +import {IERC20} from "IERC20/IERC20.sol"; +import {SafeERC20} from "SafeERC20/SafeERC20.sol"; + +error PaymentFailure(address target); + +//Note: Always forwards all gas, so consider gas griefing attack opportunities by the recipient. +//Note: Don't use this method if you need events for 0 amount transfers. +function tokenOrNativeTransfer(address tokenOrZeroForNative, address to, uint256 amount) { + if (amount == 0) + return; + + if (tokenOrZeroForNative == address(0)) { + (bool success, ) = to.call{value: amount}(new bytes(0)); + if (!success) + revert PaymentFailure(to); + } + else + SafeERC20.safeTransfer(IERC20(tokenOrZeroForNative), to, amount); +} diff --git a/src/utils/UniversalAddress.sol b/src/utils/UniversalAddress.sol new file mode 100644 index 0000000..eb607f1 --- /dev/null +++ b/src/utils/UniversalAddress.sol @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: Apache 2 +pragma solidity ^0.8.19; + +error NotAnEvmAddress(bytes32); + +function toUniversalAddress(address addr) pure returns (bytes32 universalAddr) { + universalAddr = bytes32(uint256(uint160(addr))); +} + +function fromUniversalAddress(bytes32 universalAddr) pure returns (address addr) { + if (bytes12(universalAddr) != 0) + revert NotAnEvmAddress(universalAddr); + + assembly ("memory-safe") { + addr := universalAddr + } +} diff --git a/test/dispatcherComponents/AccessControl.t.sol b/test/components/dispatcher/AccessControl.t.sol similarity index 95% rename from test/dispatcherComponents/AccessControl.t.sol rename to test/components/dispatcher/AccessControl.t.sol index 435872f..3a2d3bd 100644 --- a/test/dispatcherComponents/AccessControl.t.sol +++ b/test/components/dispatcher/AccessControl.t.sol @@ -2,10 +2,22 @@ pragma solidity ^0.8.4; -import { NotAuthorized } from "wormhole-sdk/dispatcherComponents/AccessControl.sol"; -import { BytesParsing } from "wormhole-sdk/libraries/BytesParsing.sol"; -import { DispatcherTestBase } from "./utils/DispatcherTestBase.sol"; -import "wormhole-sdk/dispatcherComponents/ids.sol"; +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {NotAuthorized} from "wormhole-sdk/components/dispatcher/AccessControl.sol"; +import { + ACCESS_CONTROL_ID, + ACCESS_CONTROL_QUERIES_ID, + OWNER_ID, + PENDING_OWNER_ID, + ACQUIRE_OWNERSHIP_ID, + IS_ADMIN_ID, + ADMINS_ID, + REVOKE_ADMIN_ID, + ADD_ADMIN_ID, + PROPOSE_OWNERSHIP_TRANSFER_ID, + RELINQUISH_OWNERSHIP_ID +} from "wormhole-sdk/components/dispatcher/Ids.sol"; +import {DispatcherTestBase} from "./utils/DispatcherTestBase.sol"; contract AcessControlTest is DispatcherTestBase { using BytesParsing for bytes; diff --git a/test/dispatcherComponents/SweepTokens.t.sol b/test/components/dispatcher/SweepTokens.t.sol similarity index 85% rename from test/dispatcherComponents/SweepTokens.t.sol rename to test/components/dispatcher/SweepTokens.t.sol index 0fb1d96..f57efd9 100644 --- a/test/dispatcherComponents/SweepTokens.t.sol +++ b/test/components/dispatcher/SweepTokens.t.sol @@ -2,9 +2,10 @@ pragma solidity ^0.8.4; -import {SWEEP_TOKENS_ID} from "wormhole-sdk/dispatcherComponents/ids.sol"; +import {SWEEP_TOKENS_ID} from "wormhole-sdk/components/dispatcher/Ids.sol"; +import {UpgradeTester} from "wormhole-sdk/testing/UpgradeTester.sol"; +import {ERC20Mock} from "wormhole-sdk/testing/ERC20Mock.sol"; import {DispatcherTestBase} from "./utils/DispatcherTestBase.sol"; -import {ERC20Mock} from "wormhole-sdk/testing/ERC20Mock.sol"; contract SweepTokensTest is DispatcherTestBase { ERC20Mock token; diff --git a/test/dispatcherComponents/Upgrade.t.sol b/test/components/dispatcher/Upgrade.t.sol similarity index 87% rename from test/dispatcherComponents/Upgrade.t.sol rename to test/components/dispatcher/Upgrade.t.sol index b3c50de..264292f 100644 --- a/test/dispatcherComponents/Upgrade.t.sol +++ b/test/components/dispatcher/Upgrade.t.sol @@ -2,12 +2,15 @@ pragma solidity ^0.8.4; -import {NotAuthorized} from "wormhole-sdk/dispatcherComponents/AccessControl.sol"; -import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; -import {UpgradeTester} from "wormhole-sdk/testing/UpgradeTester.sol"; -import {IdempotentUpgrade} from "wormhole-sdk/proxy/ProxyBase.sol"; -import {DispatcherTestBase} from "./utils/DispatcherTestBase.sol"; -import "wormhole-sdk/dispatcherComponents/ids.sol"; +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {IdempotentUpgrade} from "wormhole-sdk/proxy/ProxyBase.sol"; +import {NotAuthorized} from "wormhole-sdk/components/dispatcher/AccessControl.sol"; +import { + UPGRADE_CONTRACT_ID, + IMPLEMENTATION_ID +} from "wormhole-sdk/components/dispatcher/Ids.sol"; +import {UpgradeTester} from "wormhole-sdk/testing/UpgradeTester.sol"; +import {DispatcherTestBase} from "./utils/DispatcherTestBase.sol"; contract UpgradeTest is DispatcherTestBase { using BytesParsing for bytes; diff --git a/test/dispatcherComponents/utils/Dispatcher.sol b/test/components/dispatcher/utils/Dispatcher.sol similarity index 86% rename from test/dispatcherComponents/utils/Dispatcher.sol rename to test/components/dispatcher/utils/Dispatcher.sol index 8224877..973f80a 100644 --- a/test/dispatcherComponents/utils/Dispatcher.sol +++ b/test/components/dispatcher/utils/Dispatcher.sol @@ -2,11 +2,11 @@ pragma solidity ^0.8.4; -import {AccessControl} from "wormhole-sdk/dispatcherComponents/AccessControl.sol"; -import {SweepTokens} from "wormhole-sdk/dispatcherComponents/SweepTokens.sol"; -import {Upgrade} from "wormhole-sdk/dispatcherComponents/Upgrade.sol"; -import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; import {RawDispatcher} from "wormhole-sdk/RawDispatcher.sol"; +import {AccessControl} from "wormhole-sdk/components/dispatcher/AccessControl.sol"; +import {SweepTokens} from "wormhole-sdk/components/dispatcher/SweepTokens.sol"; +import {Upgrade} from "wormhole-sdk/components/dispatcher/Upgrade.sol"; contract Dispatcher is RawDispatcher, AccessControl, SweepTokens, Upgrade { using BytesParsing for bytes; diff --git a/test/dispatcherComponents/utils/DispatcherTestBase.sol b/test/components/dispatcher/utils/DispatcherTestBase.sol similarity index 91% rename from test/dispatcherComponents/utils/DispatcherTestBase.sol rename to test/components/dispatcher/utils/DispatcherTestBase.sol index 59f067a..6ad46bc 100644 --- a/test/dispatcherComponents/utils/DispatcherTestBase.sol +++ b/test/components/dispatcher/utils/DispatcherTestBase.sol @@ -2,12 +2,13 @@ pragma solidity ^0.8.4; -import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; -import {Proxy} from "wormhole-sdk/proxy/Proxy.sol"; -import {reRevert} from "wormhole-sdk/Utils.sol"; -import {Dispatcher} from "./Dispatcher.sol"; import "forge-std/Test.sol"; +import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; +import {Proxy} from "wormhole-sdk/proxy/Proxy.sol"; +import {reRevert} from "wormhole-sdk/Utils.sol"; +import {Dispatcher} from "./Dispatcher.sol"; + contract DispatcherTestBase is Test { using BytesParsing for bytes; From 39c772aa021ef143ed14805417fbd27b6fa11dd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Claudio=20Nale?= Date: Tue, 19 Nov 2024 09:26:42 -0300 Subject: [PATCH 3/9] Removes unused openzeppelin submodule --- .gitmodules | 5 +---- lib/openzeppelin-contracts | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) delete mode 160000 lib/openzeppelin-contracts diff --git a/.gitmodules b/.gitmodules index 690924b..4c1b977 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,6 +1,3 @@ [submodule "lib/forge-std"] path = lib/forge-std - url = https://github.com/foundry-rs/forge-std -[submodule "lib/openzeppelin-contracts"] - path = lib/openzeppelin-contracts - url = https://github.com/OpenZeppelin/openzeppelin-contracts + url = https://github.com/foundry-rs/forge-std \ No newline at end of file diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts deleted file mode 160000 index 3291252..0000000 --- a/lib/openzeppelin-contracts +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 3291252c866ad698f6a55ec660259e49a67eb3d0 From 50862c305e2cafaa6ff9a0cc327165b021486aab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Claudio=20Nale?= Date: Tue, 19 Nov 2024 12:57:48 -0300 Subject: [PATCH 4/9] Simplifies test dispatcher invoke functions --- .../dispatcher/utils/DispatcherTestBase.sol | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/test/components/dispatcher/utils/DispatcherTestBase.sol b/test/components/dispatcher/utils/DispatcherTestBase.sol index 6ad46bc..c0a0929 100644 --- a/test/components/dispatcher/utils/DispatcherTestBase.sol +++ b/test/components/dispatcher/utils/DispatcherTestBase.sol @@ -43,28 +43,22 @@ contract DispatcherTestBase is Test { function invokeStaticDispatcher(bytes memory encoded) view internal returns (bytes memory data) { (bool success, bytes memory result) = address(dispatcher).staticcall(encoded); - if (!success) { - reRevert(result); - } - (uint length,) = result.asUint256Unchecked(32); - (data,) = result.sliceUnchecked(64, length); + return decodeBytesResult(success, result); } function invokeDispatcher(bytes memory encoded) internal returns (bytes memory data) { - (bool success, bytes memory result) = address(dispatcher).call(encoded); - if (!success) { - reRevert(result); - } - (uint length,) = result.asUint256Unchecked(32); - (data,) = result.sliceUnchecked(64, length); + return invokeDispatcher(encoded, 0); } function invokeDispatcher(bytes memory encoded, uint value) internal returns (bytes memory data) { (bool success, bytes memory result) = address(dispatcher).call{value: value}(encoded); + return decodeBytesResult(success, result); + } + + function decodeBytesResult(bool success, bytes memory result) pure private returns (bytes memory data) { if (!success) { reRevert(result); } - (uint length,) = result.asUint256Unchecked(32); - (data,) = result.sliceUnchecked(64, length); + data = abi.decode(result, (bytes)); } } \ No newline at end of file From e33002f89e745ab725cbc3a00c730fb740772179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Claudio=20Nale?= Date: Wed, 20 Nov 2024 14:41:09 -0300 Subject: [PATCH 5/9] Simplifies contract invocations in tests --- .../components/dispatcher/AccessControl.t.sol | 31 ------------------- test/components/dispatcher/SweepTokens.t.sol | 2 -- test/components/dispatcher/Upgrade.t.sol | 8 ----- .../dispatcher/utils/DispatcherTestBase.sol | 14 +++++---- 4 files changed, 8 insertions(+), 47 deletions(-) diff --git a/test/components/dispatcher/AccessControl.t.sol b/test/components/dispatcher/AccessControl.t.sol index 3a2d3bd..611a397 100644 --- a/test/components/dispatcher/AccessControl.t.sol +++ b/test/components/dispatcher/AccessControl.t.sol @@ -29,7 +29,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, PROPOSE_OWNERSHIP_TRANSFER_ID, @@ -40,7 +39,6 @@ contract AcessControlTest is DispatcherTestBase { commandCount = 2; bytes memory getRes = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, OWNER_ID, @@ -56,14 +54,12 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(newOwner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACQUIRE_OWNERSHIP_ID ) ); getRes = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, @@ -85,7 +81,6 @@ contract AcessControlTest is DispatcherTestBase { vm.expectRevert(NotAuthorized.selector); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, PROPOSE_OWNERSHIP_TRANSFER_ID, @@ -101,7 +96,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, PROPOSE_OWNERSHIP_TRANSFER_ID, @@ -112,7 +106,6 @@ contract AcessControlTest is DispatcherTestBase { commandCount = 2; bytes memory getRes = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, OWNER_ID, @@ -134,7 +127,6 @@ contract AcessControlTest is DispatcherTestBase { vm.expectRevert(NotAuthorized.selector); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACQUIRE_OWNERSHIP_ID ) ); @@ -147,7 +139,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, PROPOSE_OWNERSHIP_TRANSFER_ID, @@ -158,14 +149,12 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(newOwner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACQUIRE_OWNERSHIP_ID ) ); bytes memory getRes = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, @@ -194,7 +183,6 @@ contract AcessControlTest is DispatcherTestBase { uint8 commandCount = 2; bytes memory getRes = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, OWNER_ID, @@ -223,7 +211,6 @@ contract AcessControlTest is DispatcherTestBase { uint8 commandCount = 2; bytes memory getRes = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, OWNER_ID, @@ -252,7 +239,6 @@ contract AcessControlTest is DispatcherTestBase { uint8 commandCount = 2; bytes memory getRes = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, OWNER_ID, @@ -276,7 +262,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, PROPOSE_OWNERSHIP_TRANSFER_ID, @@ -287,7 +272,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(newOwner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACQUIRE_OWNERSHIP_ID, ACCESS_CONTROL_ID, commandCount, @@ -299,7 +283,6 @@ contract AcessControlTest is DispatcherTestBase { commandCount = 3; bytes memory getRes = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, OWNER_ID, @@ -328,7 +311,6 @@ contract AcessControlTest is DispatcherTestBase { vm.expectRevert(NotAuthorized.selector); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, ADD_ADMIN_ID, @@ -340,7 +322,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(admin); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, ADD_ADMIN_ID, @@ -356,7 +337,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, ADD_ADMIN_ID, @@ -367,7 +347,6 @@ contract AcessControlTest is DispatcherTestBase { commandCount = 2; bytes memory res = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, IS_ADMIN_ID, @@ -392,7 +371,6 @@ contract AcessControlTest is DispatcherTestBase { vm.expectRevert(NotAuthorized.selector); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, REVOKE_ADMIN_ID, @@ -408,7 +386,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, ADD_ADMIN_ID, @@ -420,7 +397,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(admin); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, REVOKE_ADMIN_ID, @@ -431,7 +407,6 @@ contract AcessControlTest is DispatcherTestBase { commandCount = 2; bytes memory res = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, IS_ADMIN_ID, @@ -453,7 +428,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(admin); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, REVOKE_ADMIN_ID, @@ -464,7 +438,6 @@ contract AcessControlTest is DispatcherTestBase { bool isAdmin; (isAdmin, ) = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, IS_ADMIN_ID, @@ -481,7 +454,6 @@ contract AcessControlTest is DispatcherTestBase { vm.expectRevert(NotAuthorized.selector); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, RELINQUISH_OWNERSHIP_ID @@ -498,7 +470,6 @@ contract AcessControlTest is DispatcherTestBase { ); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, RELINQUISH_OWNERSHIP_ID, @@ -514,7 +485,6 @@ contract AcessControlTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, ACCESS_CONTROL_ID, commandCount, RELINQUISH_OWNERSHIP_ID @@ -523,7 +493,6 @@ contract AcessControlTest is DispatcherTestBase { (address owner_, ) = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, ACCESS_CONTROL_QUERIES_ID, commandCount, OWNER_ID diff --git a/test/components/dispatcher/SweepTokens.t.sol b/test/components/dispatcher/SweepTokens.t.sol index f57efd9..e083306 100644 --- a/test/components/dispatcher/SweepTokens.t.sol +++ b/test/components/dispatcher/SweepTokens.t.sol @@ -24,7 +24,6 @@ contract SweepTokensTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, SWEEP_TOKENS_ID, address(token), tokenAmount ) ); @@ -42,7 +41,6 @@ contract SweepTokensTest is DispatcherTestBase { vm.prank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, SWEEP_TOKENS_ID, address(0), ethAmount ) ); diff --git a/test/components/dispatcher/Upgrade.t.sol b/test/components/dispatcher/Upgrade.t.sol index 264292f..6ffa59c 100644 --- a/test/components/dispatcher/Upgrade.t.sol +++ b/test/components/dispatcher/Upgrade.t.sol @@ -21,7 +21,6 @@ contract UpgradeTest is DispatcherTestBase { vm.expectRevert(NotAuthorized.selector); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, UPGRADE_CONTRACT_ID, address(fakeAddress) ) @@ -31,7 +30,6 @@ contract UpgradeTest is DispatcherTestBase { vm.expectRevert(NotAuthorized.selector); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, UPGRADE_CONTRACT_ID, address(fakeAddress) ) @@ -44,7 +42,6 @@ contract UpgradeTest is DispatcherTestBase { vm.startPrank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, UPGRADE_CONTRACT_ID, address(upgradeTester) ) @@ -59,7 +56,6 @@ contract UpgradeTest is DispatcherTestBase { bytes memory response = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, IMPLEMENTATION_ID ) ); @@ -69,7 +65,6 @@ contract UpgradeTest is DispatcherTestBase { vm.startPrank(owner); invokeDispatcher( abi.encodePacked( - dispatcher.exec768.selector, UPGRADE_CONTRACT_ID, address(upgradeTester) ) @@ -79,7 +74,6 @@ contract UpgradeTest is DispatcherTestBase { response = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, IMPLEMENTATION_ID ) ); @@ -114,7 +108,6 @@ contract UpgradeTest is DispatcherTestBase { bytes memory response = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, IMPLEMENTATION_ID ) ); @@ -128,7 +121,6 @@ contract UpgradeTest is DispatcherTestBase { response = invokeStaticDispatcher( abi.encodePacked( - dispatcher.get1959.selector, IMPLEMENTATION_ID ) ); diff --git a/test/components/dispatcher/utils/DispatcherTestBase.sol b/test/components/dispatcher/utils/DispatcherTestBase.sol index c0a0929..38dfe4e 100644 --- a/test/components/dispatcher/utils/DispatcherTestBase.sol +++ b/test/components/dispatcher/utils/DispatcherTestBase.sol @@ -41,17 +41,19 @@ contract DispatcherTestBase is Test { _setUp1(); } - function invokeStaticDispatcher(bytes memory encoded) view internal returns (bytes memory data) { - (bool success, bytes memory result) = address(dispatcher).staticcall(encoded); + function invokeStaticDispatcher(bytes memory messages) view internal returns (bytes memory data) { + bytes memory getCall = abi.encodePacked(dispatcher.get1959.selector, messages); + (bool success, bytes memory result) = address(dispatcher).staticcall(getCall); return decodeBytesResult(success, result); } - function invokeDispatcher(bytes memory encoded) internal returns (bytes memory data) { - return invokeDispatcher(encoded, 0); + function invokeDispatcher(bytes memory messages) internal returns (bytes memory data) { + return invokeDispatcher(messages, 0); } - function invokeDispatcher(bytes memory encoded, uint value) internal returns (bytes memory data) { - (bool success, bytes memory result) = address(dispatcher).call{value: value}(encoded); + function invokeDispatcher(bytes memory messages, uint value) internal returns (bytes memory data) { + bytes memory execCall = abi.encodePacked(dispatcher.exec768.selector, messages); + (bool success, bytes memory result) = address(dispatcher).call{value: value}(execCall); return decodeBytesResult(success, result); } From 4eb350050243217003acf00f709266d3acf7e6bc Mon Sep 17 00:00:00 2001 From: scnale Date: Thu, 21 Nov 2024 12:01:37 -0300 Subject: [PATCH 6/9] fix: typo in README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b4166c6..4193b30 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,7 @@ It is strongly recommended that you run the forge test suite of this SDK with yo **IERC20 and SafeERC20 Remapping** -This SDK comes with its own IERC20 interface and SafeERC20 implementation. Given that projects tend to combine different SDKs, there's often this annoying issue of clashes of IERC20 interfaces, even though the are effectively the same. We handle this issue by importing `IERC20/IERC20.sol` which allows remapping the `IERC20/` prefix to whatever directory contains `IERC20.sol` in your project, thus providing an override mechanism that should allow dealing with this problem seamlessly until forge allows remapping of individual files. The same approach is used for SafeERC20. +This SDK comes with its own IERC20 interface and SafeERC20 implementation. Given that projects tend to combine different SDKs, there's often this annoying issue of clashes of IERC20 interfaces, even though they are effectively the same. We handle this issue by importing `IERC20/IERC20.sol` which allows remapping the `IERC20/` prefix to whatever directory contains `IERC20.sol` in your project, thus providing an override mechanism that should allow dealing with this problem seamlessly until forge allows remapping of individual files. The same approach is used for SafeERC20. ## Components From 704ffa33663fa55deaa48b03f403bbb6bfcf7548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Claudio=20Nale?= Date: Thu, 21 Nov 2024 17:08:09 -0300 Subject: [PATCH 7/9] Adds a few event checks --- .../components/dispatcher/AccessControl.t.sol | 29 ++++++++++++++----- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/test/components/dispatcher/AccessControl.t.sol b/test/components/dispatcher/AccessControl.t.sol index 611a397..1c6c7ac 100644 --- a/test/components/dispatcher/AccessControl.t.sol +++ b/test/components/dispatcher/AccessControl.t.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.4; import {BytesParsing} from "wormhole-sdk/libraries/BytesParsing.sol"; -import {NotAuthorized} from "wormhole-sdk/components/dispatcher/AccessControl.sol"; +import {AdminsUpdated, NotAuthorized} from "wormhole-sdk/components/dispatcher/AccessControl.sol"; import { ACCESS_CONTROL_ID, ACCESS_CONTROL_QUERIES_ID, @@ -334,6 +334,8 @@ contract AcessControlTest is DispatcherTestBase { vm.assume(newAdmin != admin); uint8 commandCount = 1; + vm.expectEmit(); + emit AdminsUpdated(newAdmin, true, block.timestamp); vm.prank(owner); invokeDispatcher( abi.encodePacked( @@ -394,6 +396,10 @@ contract AcessControlTest is DispatcherTestBase { ); commandCount = 1; + + vm.expectEmit(); + emit AdminsUpdated(newAdmin, false, block.timestamp); + vm.prank(admin); invokeDispatcher( abi.encodePacked( @@ -425,6 +431,9 @@ contract AcessControlTest is DispatcherTestBase { function testRelinquishAdministration() public { uint8 commandCount = 1; + vm.expectEmit(); + emit AdminsUpdated(admin, false, block.timestamp); + vm.prank(admin); invokeDispatcher( abi.encodePacked( @@ -450,15 +459,19 @@ contract AcessControlTest is DispatcherTestBase { function testRelinquishOwnership_NotAuthorized() public { uint8 commandCount = 1; + bytes memory relinquishCommand = abi.encodePacked( + ACCESS_CONTROL_ID, + commandCount, + RELINQUISH_OWNERSHIP_ID + ); vm.expectRevert(NotAuthorized.selector); - invokeDispatcher( - abi.encodePacked( - ACCESS_CONTROL_ID, - commandCount, - RELINQUISH_OWNERSHIP_ID - ) - ); + invokeDispatcher(relinquishCommand); + + + vm.expectRevert(NotAuthorized.selector); + vm.prank(admin); + invokeDispatcher(relinquishCommand); } function testRelinquishOwnership_LengthMismatch() public { From 2d9c21522fbbaf913388e14438c83e79372c8a3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Claudio=20Nale?= Date: Thu, 21 Nov 2024 18:18:41 -0300 Subject: [PATCH 8/9] Further simplification of `AccessControl` tests --- .../components/dispatcher/AccessControl.t.sol | 51 +++++++------------ 1 file changed, 17 insertions(+), 34 deletions(-) diff --git a/test/components/dispatcher/AccessControl.t.sol b/test/components/dispatcher/AccessControl.t.sol index 1c6c7ac..d548053 100644 --- a/test/components/dispatcher/AccessControl.t.sol +++ b/test/components/dispatcher/AccessControl.t.sol @@ -37,14 +37,13 @@ contract AcessControlTest is DispatcherTestBase { ); commandCount = 2; - bytes memory getRes = invokeStaticDispatcher( - abi.encodePacked( - ACCESS_CONTROL_QUERIES_ID, - commandCount, - OWNER_ID, - PENDING_OWNER_ID - ) - ); + bytes memory queries = abi.encodePacked( + ACCESS_CONTROL_QUERIES_ID, + commandCount, + OWNER_ID, + PENDING_OWNER_ID + ) + bytes memory getRes = invokeStaticDispatcher(queries); (address owner_, ) = getRes.asAddressUnchecked(0); (address pendingOwner_, ) = getRes.asAddressUnchecked(20); @@ -58,15 +57,7 @@ contract AcessControlTest is DispatcherTestBase { ) ); - getRes = invokeStaticDispatcher( - abi.encodePacked( - - ACCESS_CONTROL_QUERIES_ID, - commandCount, - OWNER_ID, - PENDING_OWNER_ID - ) - ); + getRes = invokeStaticDispatcher(queries); (owner_, ) = getRes.asAddressUnchecked(0); (pendingOwner_, ) = getRes.asAddressUnchecked(20); @@ -155,7 +146,6 @@ contract AcessControlTest is DispatcherTestBase { bytes memory getRes = invokeStaticDispatcher( abi.encodePacked( - ACCESS_CONTROL_QUERIES_ID, commandCount, OWNER_ID, @@ -308,26 +298,19 @@ contract AcessControlTest is DispatcherTestBase { address newAdmin = makeAddr("newAdmin"); uint8 commandCount = 1; - vm.expectRevert(NotAuthorized.selector); - invokeDispatcher( - abi.encodePacked( - ACCESS_CONTROL_ID, - commandCount, - ADD_ADMIN_ID, - newAdmin - ) + bytes memory = addAdminCommand = abi.encodePacked( + ACCESS_CONTROL_ID, + commandCount, + ADD_ADMIN_ID, + newAdmin ); + vm.expectRevert(NotAuthorized.selector); + invokeDispatcher(addAdminCommand); + vm.expectRevert(NotAuthorized.selector); vm.prank(admin); - invokeDispatcher( - abi.encodePacked( - ACCESS_CONTROL_ID, - commandCount, - ADD_ADMIN_ID, - newAdmin - ) - ); + invokeDispatcher(addAdminCommand); } function testAddAdmin(address newAdmin) public { From 914da355cd841ae8c3199255039edca2ea80726c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Claudio=20Nale?= Date: Fri, 22 Nov 2024 23:02:30 -0300 Subject: [PATCH 9/9] fix: syntax typos --- test/components/dispatcher/AccessControl.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/components/dispatcher/AccessControl.t.sol b/test/components/dispatcher/AccessControl.t.sol index d548053..1098a5a 100644 --- a/test/components/dispatcher/AccessControl.t.sol +++ b/test/components/dispatcher/AccessControl.t.sol @@ -42,7 +42,7 @@ contract AcessControlTest is DispatcherTestBase { commandCount, OWNER_ID, PENDING_OWNER_ID - ) + ); bytes memory getRes = invokeStaticDispatcher(queries); (address owner_, ) = getRes.asAddressUnchecked(0); (address pendingOwner_, ) = getRes.asAddressUnchecked(20); @@ -298,7 +298,7 @@ contract AcessControlTest is DispatcherTestBase { address newAdmin = makeAddr("newAdmin"); uint8 commandCount = 1; - bytes memory = addAdminCommand = abi.encodePacked( + bytes memory addAdminCommand = abi.encodePacked( ACCESS_CONTROL_ID, commandCount, ADD_ADMIN_ID,