add proxy contracts to simplify upgradability (#51)

* add proxy contracts to simplify upgradability

* implement proxy tests, harden code, add/update READMEs

* emphasize exclusive storage slot pattern usage in proxy README

Author: nonergodic

README.md

@@ -26,9 +26,20 @@ The `main` branch is considered the nightly version of the SDK. Stick to tagged
 forge install wormhole-foundation/[email protected]
 ```
 
-**Solc Version**
+**EVM Version**
 
-Currently the SDK uses solc version 0.8.19 to avoid issues with PUSH0 which was introduced in 0.8.20 but which is not supported on many EVM chains.
+One hazard of developing EVM contracts in a cross-chain environment is that different chains have varying levels EVM-equivalence. This means you have to ensure that all chains that you are planning to deploy to  support all EIPs/opcodes that you rely on.
+
+For example, if you are using a solc version newer than `0.8.19` and are planning to deploy to a chain that does not support [PUSH0 opcode](https://eips.ethereum.org/EIPS/eip-3855) (introduced as part of the Shanghai hardfork), you should set `evm_version = "paris"` in your `foundry.toml`, since the default EVM version of solc was advanced from Paris to Shanghai as part of solc's `0.8.20` release.
+
+## Philosophy/Creeds
+
+In This House We Believe:
+* clarity breeds security
+* Do NOT trust in the Lord (i.e. the community, auditors, fellow devs, FOSS, ...) with any of your heart (i.e. with your or your users' security), but lean _hard_ on your own understanding.
+* _Nothing_ is considered safe unless you have _personally_ verified it as such.
+* git gud
+* shut up and suffer
 
 ## WormholeRelayer
 

foundry.toml

@@ -1,5 +1,6 @@
 [profile.default]
-solc_version = "0.8.19"
+solc_version = "0.8.24"
+evm_version = "paris" # prevent use of PUSH0 opcode until it is widely supported
 src = "src"
 out = "out"
 libs = ["lib"]

src/proxy/Eip1967Admin.sol

@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: Apache 2
+
+pragma solidity ^0.8.24;
+
+// optional default implementation of eip1967 admin storage
+//
+// examples of natural extensions/overrides are:
+//  - additional pendingAdmin for 2-step ownership transfers
+//  - storing additional roles (after the admin slot)
+
+struct AdminState {
+  address admin;
+}
+
+// we use the designated eip1967 admin storage slot: keccak256("eip1967.proxy.admin") - 1
+bytes32 constant ADMIN_SLOT =
+  0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
+
+function adminState() pure returns (AdminState storage state) {
+  assembly ("memory-safe") { state.slot := ADMIN_SLOT }
+}

src/proxy/Eip1967Implementation.sol

@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: Apache 2
+
+pragma solidity ^0.8.24;
+
+struct ImplementationState {
+  address implementation;
+  bool    initialized;
+}
+
+// we use the designated eip1967 storage slot: keccak256("eip1967.proxy.implementation") - 1
+bytes32 constant IMPLEMENTATION_SLOT =
+  0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
+
+function implementationState() pure returns (ImplementationState storage state) {
+  assembly ("memory-safe") { state.slot := IMPLEMENTATION_SLOT }
+}

src/proxy/Proxy.sol

@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: Apache 2
+
+pragma solidity ^0.8.24;
+
+import { implementationState } from "./Eip1967Implementation.sol";
+
+error ProxyConstructionFailed(bytes revertData);
+
+//slimmed down, more opinionated implementation of the EIP1967 reference implementation
+//  see: https://eips.ethereum.org/EIPS/eip-1967
+contract Proxy {
+  constructor(address logic, bytes memory data) payable {
+    implementationState().implementation = logic;
+
+    //We can't externally call ourselves and use msg.sender to prevent unauhorized execution of
+    //  the construction code, because the proxy's code only gets written to state when the
+    //  deployment transaction completes (and returns the deployed bytecode via CODECOPY).
+    //So we only have delegatecall at our disposal and instead use an initialized flag (stored in
+    //  the same storage slot as the implementation address) to prevent invalid re-initialization.
+    (bool success, bytes memory revertData) =
+      logic.delegatecall(abi.encodeWithSignature("checkedUpgrade(bytes)", (data)));
+
+    if (!success)
+      revert ProxyConstructionFailed(revertData);
+  }
+
+  fallback() external payable {
+    //can't just do a naked sload of the implementation slot here because it also contains
+    //  the initialized flag!
+    address implementation = implementationState().implementation;
+    assembly {
+      calldatacopy(0, 0, calldatasize())
+      let result := delegatecall(gas(), implementation, 0, calldatasize(), 0, 0)
+      returndatacopy(0, 0, returndatasize())
+      switch result
+      case 0 {
+        revert(0, returndatasize())
+      }
+      default {
+        return(0, returndatasize())
+      }
+    }
+  }
+}

src/proxy/ProxyBase.sol

@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: Apache 2
+
+pragma solidity ^0.8.24;
+
+import { implementationState } from "./Eip1967Implementation.sol";
+
+error InvalidSender();
+error IdempotentUpgrade();
+error InvalidMsgValue();
+error InvalidData();
+error UpgradeFailed(bytes revertData);
+
+event Upgraded(address indexed implementation);
+
+//works with both standard EIP1967 proxies and our own, slimmed down Proxy contract
+abstract contract ProxyBase {
+  //address private immutable _logicContract = address(this);
+
+  //payable for proxyConstructor use case
+  //selector: f4189c473
+  function checkedUpgrade(bytes calldata data) payable external {
+    if (msg.sender != address(this)) {
+      if (implementationState().initialized)
+        revert InvalidSender();
+
+      _proxyConstructor(data);
+    }
+    else
+      _contractUpgrade(data);
+
+    //If we upgrade from an old OpenZeppelin proxy, then initialized will not have been set to true
+    //  even though the constructor has been called, so we simply manually set it here in all cases.
+    //This is slightly gas inefficient but better to be safe than sorry for rare use cases like
+    //  contract upgrades.
+    implementationState().initialized = true;
+  }
+
+  //msg.value should be enforced/checked before calling _upgradeTo
+  function _upgradeTo(address newImplementation, bytes memory data) internal {
+    if (newImplementation == implementationState().implementation)
+      revert IdempotentUpgrade();
+
+    implementationState().implementation = newImplementation;
+
+    (bool success, bytes memory revertData) =
+      address(this).call(abi.encodeCall(this.checkedUpgrade, (data)));
+
+    if (!success)
+      revert UpgradeFailed(revertData);
+
+    emit Upgraded(newImplementation);
+  }
+
+  function _getImplementation() internal view returns (address) {
+    return implementationState().implementation;
+  }
+
+  function _proxyConstructor(bytes calldata data) internal virtual {
+    if (msg.value > 0)
+      revert InvalidMsgValue();
+
+    _noDataAllowed(data);
+
+    //!!don't forget to check/enforce msg.value when overriding!!
+  }
+
+  function _contractUpgrade(bytes calldata data) internal virtual {
+    _noDataAllowed(data);
+
+    //override and implement in the new logic contract (if required)
+  }
+
+  function _noDataAllowed(bytes calldata data) internal pure {
+    if (data.length > 0)
+      revert InvalidData();
+  }
+}

src/proxy/README.md

@@ -0,0 +1,84 @@
+# Proxy
+
+An opinionated, minimalist, efficient implementation for contract upgradability.
+
+Based on [ERC-1967](https://eips.ethereum.org/EIPS/eip-1967), but skips all features that aren't strictly necessary for the most basic upgradeability use case (such as beacon, rollback, and admin slots/functionality) so as to provide the slimmest (in terms of bytecode), most straight-forward (in terms of readability and usability), no-nonsense solution.
+
+I'm using the term "logic contract" here because it is a lot clearer/unambiguous than the more generic "implementation", while the actual code uses implementation to stick with the established terms within the context.
+
+## Usage in a Nutshell
+
+See test/Proxy.t.sol for a straight-forward example.
+
+### Implementation
+
+1. Implement a (logic) contract that inherits from `ProxyBase`.
+2. Implement a constructor to initalize all `immutable` variables of your contract.
+3. Only use the slot pattern for a storage variables. Normal storage variables are of the devil.
+4. Override `_proxyConstructor` as necessary to initialize all storage variables of your contract as necessary (and don't forget to manually check `msg.value` since the constructor of `Proxy` is `payable`!).
+5. Implement a function with a name (and access restrictions!) of your choosing that calls `_upgradeTo` internally. Make `payable` and emit an event as necessary/desired.
+
+### Deployment
+
+1. Deploy the your logic contract via its constructor.
+2. Deploy `Proxy` with the address of your logic contract and `bytes` as required by `_proxyConstructor`.
+Alternatively to step 2. you can also deploy a standard `ERC1967Proxy` (or through an `ERC1967Proxy` factory), in which case you have to manually encode the call to `checkedUpgrade`.
+
+### Upgrade
+
+1. Override `_contractUpgrade` in your new version of the logic contract and implement all migration logic there.
+2. Invoke the upgrade through your own upgrade function (step 4 in the Implementation section).
+
+## Rationale
+
+There are enough upgradability standards, patterns, and libraries out there to make anyone's head spin:
+* [ERC-1967](https://eips.ethereum.org/EIPS/eip-1967)
+* [UUPS ERC-1822](https://eips.ethereum.org/EIPS/eip-1822)
+* [Diamond pattern ERC-2535](https://eips.ethereum.org/EIPS/eip-2535)
+
+And then there's a bunch of implementations in the various EVM SDK repos:
+* [OpenZeppelin](https://github.com/OpenZeppelin/openzeppelin-contracts)
+* [solmate](https://github.com/transmissions11/solmate)
+* [solady](https://github.com/Vectorized/solady)
+
+And especially OZ, which is very commonly used, has a bunch of patterns on top of that, requiring a separate [upgradeable repo](https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable) with its `init` and `init_unchained` functions, it's `initializer` and `reinitializer` modifiers, etc. etc.
+
+While these cover all conceivable use cases (and then some), their inherent complexity and sheer amount of code can easily make a reader's eyes glaze over.
+
+The goal of this proxy solution is to provide a concise, light-weight, easily understandable solution for the most common upgradeability use case.
+
+And thus [a new implementation](https://xkcd.com/927/) is born.
+
+## Design
+
+### ProxyBase
+
+Besides stripping all non-essential code, `ProxyBase` is opinionated in two ways:
+1. It separates the act of construction from the act of upgrading via two `internal` `virtual` methods called `_proxyConstructor` and `_contractUpgrade` respectively.
+2. It provides an external function called `checkedUpgrade` to execute these two methods, while automatically handling access control by automatically keeping track of an initialized flag (for construction) and by restricting external calls to `checkedUpgrade` to the proxy contract alone.
+
+Additionally, `checkedUpgrade` has a high function selector (starts with `0xf4`) which saves gas on every other external function call on the contract, since any function with a selector lower than the one that is being invoked results in a gas overhead of 22 gas andless than 5 % of functions will, on average, have a selector higher than `checkedUpgrade`'s.
+
+### Proxy
+
+`Proxy` also strips all non-essential code and is the intended pairing of `ProxyBase`, though the latter is compatible with any `ERC-1967` proxy implementation (and can therefore also be used with any proxy factories that might have already been deployed on-chain). The advantage of using `Proxy` over other proxy implementations is that one does not have to encode the full initialization function call signature in the calldata, but only has to pack the arguments for `_proxyConstructor` and pass them along with the address of the logic contract.
+
+## Limitations
+
+### No Rollback Functionality
+
+Since the upgrade mechanism isn't baked into the proxy itself but relies on the logic contract, it is possible to brick a contract with an upgrade to a faulty implementation. Simply supplying an incorrect address will not work, since the upgrade mechanism relies on the `checkedUpgrade` function to exist on the new contract, but if the upgrade mechanism of the new implementation is broken, then there's no way to roll back an upgrade.
+
+Mitigation: See "git gud" creed. When it comes to upgradability, the #1 directive is: Avoid one-way door errors.
+
+### No Version Checking
+
+If there are several version of a given contract and the upgrades are meant to be applied sequentially, depending on the migration code of the individual version, it's possible to break/brick a contract by accidentally skip one of the upgrades.
+
+Mitigation: Upgrade all your contracts whenever you release a new version. If this isn't an exceedingly rare event in the first place, perhaps you should take up a different occupation like farming, or crash test dummy.
+
+### Self-destruct
+
+If you are using the SELFDESTRUCT opcode (formerly known as SUICIDE before Ethereum's [most pointless EIP](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-6.md) got adopted) in your contract and are deploying to a chain that hasn't implemented [EIP-6780](https://eips.ethereum.org/EIPS/eip-6780) yet, you should really know what you are doing lest you are prepared to go the way of Parity Multisig.
+
+Mitigation: Always treat guns as if they are loaded, point them in a safe direction, and keep the finger off the trigger.

test/Proxy.t.sol

@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: Apache 2
+
+// forge test --match-contract QueryTest
+
+pragma solidity ^0.8.24;
+
+import "forge-std/Test.sol";
+
+import { adminState } from "wormhole-sdk/proxy/Eip1967Admin.sol";
+import { ProxyBase, UpgradeFailed, InvalidData } from "wormhole-sdk/proxy/ProxyBase.sol";
+import { Proxy, ProxyConstructionFailed } from "wormhole-sdk/proxy/Proxy.sol";
+
+error NotAuthorized();
+error NoValueAllowed();
+
+contract LogicContractV1 is ProxyBase {
+  uint256 public immutable immutableNum;
+  string public message;
+
+  constructor(uint256 num) {
+    immutableNum = num;
+  }
+
+  function _proxyConstructor(bytes calldata data) internal override {
+    if (msg.value != 0)
+      revert NoValueAllowed();
+
+    adminState().admin = msg.sender;
+    message = abi.decode(data, (string));
+  }
+
+  function getImplementation() external view returns (address) {
+    return _getImplementation();
+  }
+
+  function customUpgradeFun(address newImplementation, bytes calldata data) external {
+    if (msg.sender != adminState().admin)
+      revert NotAuthorized();
+      
+    _upgradeTo(newImplementation, data);
+  }
+}
+
+contract LogicContractV2 is LogicContractV1 {
+  constructor(uint256 num) LogicContractV1(num) {}
+
+  function _contractUpgrade(bytes calldata data) internal override {
+    message = abi.decode(data, (string));
+  }
+}
+
+contract TestProxy is Test {
+  function testProxyUpgrade() public {
+    address admin = makeAddr("admin");
+    address rando = makeAddr("rando");
+
+    address logic1 = address(new LogicContractV1(1));
+    address logic2 = address(new LogicContractV2(2));
+
+    startHoax(admin);
+    //no value allowed
+    vm.expectRevert(abi.encodeWithSelector(
+      ProxyConstructionFailed.selector,
+      abi.encodePacked(bytes4(NoValueAllowed.selector))
+    ));
+    new Proxy{value: 1 ether}(logic1, abi.encode("v1"));
+    
+    //deploy
+    LogicContractV1 contrct = LogicContractV1(address(new Proxy(logic1, abi.encode("v1"))));
+
+    assertEq(contrct.getImplementation(), logic1);
+    assertEq(contrct.immutableNum(), 1);
+    assertEq(contrct.message(), "v1");
+
+    startHoax(rando);
+    //unauthorized upgrade
+    vm.expectRevert(NotAuthorized.selector);
+    contrct.customUpgradeFun(logic2, abi.encode("v2"));
+
+    startHoax(admin);
+    //upgrade
+    contrct.customUpgradeFun(logic2, abi.encode("v2"));
+
+    assertEq(contrct.getImplementation(), logic2);
+    assertEq(contrct.immutableNum(), 2);
+    assertEq(contrct.message(), "v2");
+
+    startHoax(rando);
+    //unauthorized downgrade
+    vm.expectRevert(NotAuthorized.selector);
+    contrct.customUpgradeFun(logic1, new bytes(0));
+
+    startHoax(admin);
+    //v1 uses default _contractUpgrade implementation which reverts on any data
+    vm.expectRevert(abi.encodeWithSelector(
+      UpgradeFailed.selector,
+      abi.encodePacked(bytes4(InvalidData.selector))
+    ));
+    contrct.customUpgradeFun(logic1, abi.encode("downgrade"));
+
+    //questionable, but possible downgrade:
+    contrct.customUpgradeFun(logic1, new bytes(0));
+
+    //highlight hazards of questionable downgrade:
+    assertEq(contrct.getImplementation(), logic1);
+    assertEq(contrct.immutableNum(), 1);
+    assertEq(contrct.message(), "v2");
+  }
+}