ERC20: enable a subnet's supply to be determined by an ERC20 token in the parent. #313

raulk · 2023-12-10T23:14:19Z

Context

Various IPC users seek to create dedicated crypoeconomies backed by their own token. They'd like to launch their token on another network and inject it into their subnet as native circulating supply, so it can be used (a) to pay for gas, and (b) as the native coin for value transfers.

Contribution

This PR introduces the notion of the "supply source" at the parent subnet. This configuration object determines the kind of supply backing a child subnet. It can be "native" or "ERC20". In the latter case, the token address must be provided. We verify that the address indeed corresponds to an ERC20 token.

We also add a funcWithToken() external non-payable function to the GatewayManagerFacet. This is analogous to fund(), but for token supply (where the amount must be passed as a parameter).

We block message propagation (when the feature flag is enabled) whenever ERC20 networks are involved, as a sender, receiver, or intermediary network.

Non-goals

This PR does NOT introduce the ability to use an ERC20 token as validator stake. We don't expect that feature to be useful at all, since a major benefit of IPC is that subnets draw economic security from the value of the collateral locked, which is denominated in the root coin.

Design doc

https://www.notion.so/pl-strflt/ERC20-Native-coin-conversion-eab9267445b6411d96ec17ef02a6510e?pvs=4

TODO

Prevent horizontal subnet xnet when subnet is using ERC20 supply.
Releasing/withdrawing funds.
~~Unit tests.~~ (preferring integration tests with a real ERC20 token involved)
Integration tests (with TODOs for pending test cases)

github-actions · 2023-12-10T23:14:30Z

Your PR was set to target main. PRs should be target dev.
The base branch of this PR has been automatically changed to dev.
If you really intend to target main, edit the PR.

adlrocha · 2023-12-11T08:09:43Z

src/SubnetActorDiamond.sol

+                revert InvalidERC20Address();
+            }
+            // We require that the ERC20 token exists beforehand.
+            // The call to balanceOf will revert if the supplied address does not exist, or if it's not an ERC20 contract.


Nice trick!

src/gateway/GatewayManagerFacet.sol

dnkolegov · 2023-12-11T12:55:07Z

src/gateway/GatewayManagerFacet.sol

@@ -142,6 +147,12 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {
            // prevent spamming if there's no value to fund.
            revert InvalidCrossMsgValue();
        }
+        // Validate that the supply strategy is native.


fund description now is outdated

dnkolegov · 2023-12-11T12:57:37Z

src/gateway/GatewayManagerFacet.sol

@@ -142,6 +147,12 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {
            // prevent spamming if there's no value to fund.
            revert InvalidCrossMsgValue();
        }
+        // Validate that the supply strategy is native.
+        SupplyStrategy memory supplyStrategy = SubnetActorGetterFacet(subnetId.getActor()).supplyStrategy();


Maybe define a getter like IsSubnetWithNativeSupply ?

dnkolegov · 2023-12-11T13:00:06Z

src/gateway/GatewayManagerFacet.sol

@@ -154,6 +165,31 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {
        LibGateway.commitTopDownMsg(crossMsg);
    }

+    function fundWithToken(SubnetID calldata subnetId, FvmAddress calldata to, uint256 amount) external nonReentrant {


Should we validate the address to here? Zero address?

I don't think so? 0x00..00 is generally used as a burn address and I don't see why we should specifically prevent deposits straight into the 0x00..00 address. I'm not aware of ERC20 code that prevent such transfers in Ethereum. cc @snissn

0x0 has a "special case-ness" in ethereum and i'm not 100% familiar with all of the mechanics to it. Here's some discussion -- https://forum.openzeppelin.com/t/removing-address-0x0-checks-from-openzeppelin-contracts/2222/2

interestingly it seems to regard 0x0 as the "burn address" and wants to separate semantics around transfer and burn. we may want to consider 0x0 as our burn address and welcoming this standard

The comments in that discussion sum it up pretty well. Various standards have coalesced towards treating the 0x0 address as the creator and burn sink of things. I think blocking 0x0 at this level may hinder legitimate user flows down the line. That said, enforcing the restriction now and then loosening it up is more backwards compatible than the other way around...

Maybe worth considering consensus-shipyard/fendermint#409 has introduced 0x0 for the system address, so at least it works as a source. Hopefully the fact that it doesn't have an actual Ethereum account doesn't interfere with using it as a burn address.

The bug is that we were not incrementing the top-down nonce when inverting direction.

src/lib/SupplySourceHelper.sol

test/integration/GatewayDiamondToken.t.sol

snissn · 2023-12-16T00:34:34Z

For tests you mentioned them being incomplete, here are the tests i can think of to add:

assert fail funding a native subnet with erc20
assert fail funding an erc20 subnet with native
test using a real erc20 as opposed to a mock
test withdrawing from the subnet (is that possible?)
built hardhat entry points and makefile commands that execute code and use that as a test

adlrocha · 2023-12-18T18:10:02Z

src/gateway/GatewayMessengerFacet.sol

+        bool reject;
+        if (isLCA) {
+            // We're connecting both networks, so we check them directly.
+            reject = from.getActor().hasSupplyOfKind(SupplyKind.ERC20);


I think this is wrong, if we assume that multi-level is enabled you may be committing a message locally in a subnet, but you don't have access to the actors of the from or the to because you are an intermediate subnet and they have nothing to you with you. You only need to forward the message, but you know nothing about the source or origin subnets. It should be fine for now, but we should add a not or something to flag it and fix it in the future.

@adlrocha yep, this check only seems to catch sibling to sibling xnet comms (where the first hop is to your LCA). I'll adjust this. Thanks for catching!

On the bottom-up path or the pivot, I'd love to catch if the arrivingFrom network was an ERC20 network and reject the message then. However, unless we save the forwarder network in the postbox, we can't do this under propagate. I don't want to do it in _applyMsg because that would make a whole batch fail. I'll open an issue for this, as I don't think it's worth refactoring the postbox right now to store both the arrivingFrom and the CrossMsg.

https://github.com/consensus-shipyard/ipc-solidity-actors/issues/393

adlrocha · 2023-12-18T18:14:33Z

src/gateway/GatewayMessengerFacet.sol

+        // Else, commit a bottom up message.
+        LibGateway.commitBottomUpMsg(crossMessage);
+        // gas-opt: original check: value > 0
+        return (shouldBurn = crossMessage.message.value != 0);


This assumes that the message value includes the fee to be paid (which is currently the case, but worth noting in case we change the way it works in the future.

I don't think I changed anything here. Do you want to open an issue to track this?

adlrocha · 2023-12-18T18:31:33Z

src/lib/SupplySourceHelper.sol

+    function lock(SupplySource memory supplySource, address from, uint256 value) internal {
+        if (supplySource.kind == SupplyKind.ERC20) {
+            IERC20 token = IERC20(supplySource.tokenAddress);
+            token.safeTransferFrom({from: from, to: address(this), value: value});


This assumes a previous approve from the user to the gateway, right? Should we document it explicitly somewhere so we don't forget to add it to the docs and is clear for auditors (or anyone else reading the code?)

Yes, this is also how bridges work more generally, but agree we should document.

Added soldocs in the IGateway interface.

adlrocha

Left a few comments. Apart from something that I feel may be wrong when handling multi-level propagations (and that shouldn't be blocking) the implementation of the ERC20 side of things LGTM. I see that at least the happy path and some failure cases are covered in the tests, so should be good to go (at least for the audits)

raulk · 2023-12-18T23:20:21Z

scripts/deploy-gateway.template.ts

+/* eslint prefer-const: "off" */
 import { deployContractWithDeployer, getTransactionFees } from './util'
+import hre, { ethers } from 'hardhat'


The husky prettier hook insists on making these changes 🤷

raulk · 2023-12-18T23:21:00Z

src/lib/SupplySourceHelper.sol

+    function lock(SupplySource memory supplySource, address from, uint256 value) internal {
+        if (supplySource.kind == SupplyKind.ERC20) {
+            IERC20 token = IERC20(supplySource.tokenAddress);
+            token.safeTransferFrom({from: from, to: address(this), value: value});


Added soldocs in the IGateway interface.

raulk · 2023-12-18T23:21:30Z

test/integration/GatewayDiamondToken.t.sol

+    function test_propagation() public {
+        // TODO:
+        // 1. Test that propagation is rejected when sender is ERC20.
+        // 2. Test that propagation is rejected when receiver is ERC20.
+        // 3. Test that propagation is rejected when an intermediary subnet is ERC20.
+    }


TODO, will open an issue to track these missing tests.

Aderyn alerted of this: Passing an arbitrary `from` address to `transferFrom` (or `safeTransferFrom`) can lead to loss of funds, because anyone can transfer tokens from the `from` address if an approval is made. This is not a possibility since this parameter is in an internal library, and the only caller passes in msg.sender. But I can't predict how this code will evolve, so it may become a footgun down the line. Removing.

raulk · 2023-12-18T23:53:17Z

@snissn

assert fail funding a native subnet with erc20

assert fail funding an erc20 subnet with native

test using a real erc20 as opposed to a mock

test withdrawing from the subnet (is that possible?)

Implemented!

built hardhat entry points and makefile commands that execute code and use that as a test

Could you elaborate?

raulk · 2023-12-19T00:19:18Z

I'm seeing flaky tests failing at the diamond selector generation step, which is a Python script that calls Forge from within tests via FFI. It's flaky because it appears to fail occasionally, and with a different test each time.

I've raised a question in Slack to @dnkolegov to understand why we perform selector generation at test time, when we'll anyway need to do this during deployment or sooner.

On surface, it seems like a codegen thing at build time, so curious what other reasons led us to choose this solution.

raulk · 2023-12-19T00:19:41Z

For now, I'll go ahead and merge. dev seems to be broken anyway due to failed ABI generation and flaky tests.

github-actions bot changed the base branch from main to dev December 10, 2023 23:14

raulk force-pushed the raulk/feat/erc20 branch 4 times, most recently from fe94f85 to fe6ae46 Compare December 10, 2023 23:54

raulk requested review from adlrocha, dnkolegov and cryptoAtwill December 10, 2023 23:56

adlrocha reviewed Dec 11, 2023

View reviewed changes

aakoshh mentioned this pull request Dec 11, 2023

Killing and leaving a subnet with confirmation consensus-shipyard/ipc#39

Closed

dnkolegov reviewed Dec 11, 2023

View reviewed changes

raulk force-pushed the raulk/feat/erc20 branch 2 times, most recently from 3e930a3 to ebefde3 Compare December 14, 2023 15:30

raulk added 3 commits December 14, 2023 17:41

ERC20 supply source: initial version.

94f581e

add early check for subnet existence; fix test.

66e7a05

gateway: pass in SupplySource to xnet message exec.

00b1b67

raulk force-pushed the raulk/feat/erc20 branch from 209018f to 00b1b67 Compare December 14, 2023 17:41

raulk added 5 commits December 14, 2023 21:16

wip tests.

cd36e50

Merge branch 'dev' into raulk/feat/erc20

e626bc4

propagation: refuse to propagate if any network involved are ERC20.

7ad9dbe

cleanup.

df08205

simplify the logic for xnet routing; fix a bug.

5039643

The bug is that we were not incrementing the top-down nonce when inverting direction.

dnkolegov reviewed Dec 15, 2023

View reviewed changes

src/lib/SupplySourceHelper.sol Outdated Show resolved Hide resolved

snissn reviewed Dec 15, 2023

View reviewed changes

src/lib/SupplySourceHelper.sol Outdated Show resolved Hide resolved

snissn reviewed Dec 16, 2023

View reviewed changes

test/integration/GatewayDiamondToken.t.sol Outdated Show resolved Hide resolved

raulk added 3 commits December 18, 2023 00:49

expand tests; now also assert events.

fef2161

Merge branch 'dev' into raulk/feat/erc20

99c8053

use real ERC20 token in tests.

bc291ce

raulk added 3 commits December 18, 2023 19:19

adjust comment.

a5422ab

rename SupplySourceHelper#{call=>performCall} to prevent shadowing.

181ad95

fix compiler pragma.

10d954f

adlrocha reviewed Dec 18, 2023

View reviewed changes

raulk added 3 commits December 18, 2023 19:21

rename parameter.

5a80b23

perform ERC20 interface check with address(0).

7abf037

document SupplySource.

4079663

adlrocha reviewed Dec 18, 2023

View reviewed changes

adlrocha approved these changes Dec 18, 2023

View reviewed changes

raulk marked this pull request as ready for review December 18, 2023 18:36

raulk mentioned this pull request Dec 18, 2023

ERC20: block xnet propagation on the bottom-up path if forwarder was ERC20 consensus-shipyard/ipc#16

Closed

raulk added 6 commits December 18, 2023 20:16

drop incorrect isLCA propagation rejection check.

156e8f6

fix SCA complaints.

5c0476c

add IGateway#fundWithToken.

a6272c9

fix typo.

633c033

add more tests.

e45ba91

add a test for child to parent calls.

38374ef

raulk force-pushed the raulk/feat/erc20 branch from ac35129 to 38374ef Compare December 18, 2023 21:53

Merge branch 'dev' into raulk/feat/erc20

e85548d

raulk commented Dec 18, 2023

View reviewed changes

raulk added 3 commits December 18, 2023 23:36

fix merge after the dev branch flip-flopped on names.

3d6406e

CI: print Aderyn failure report on failure.

71eed55

raulk force-pushed the raulk/feat/erc20 branch from 2042ede to 60af071 Compare December 18, 2023 23:50

raulk merged commit c008f0e into dev Dec 19, 2023
7 of 8 checks passed

raulk deleted the raulk/feat/erc20 branch December 19, 2023 00:19

snissn mentioned this pull request Dec 19, 2023

BURNT_FUNDS_ACTOR explanation consensus-shipyard/ipc#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERC20: enable a subnet's supply to be determined by an ERC20 token in the parent. #313

ERC20: enable a subnet's supply to be determined by an ERC20 token in the parent. #313

raulk commented Dec 10, 2023 •

edited

Loading

github-actions bot commented Dec 10, 2023

adlrocha Dec 11, 2023

dnkolegov Dec 11, 2023

dnkolegov Dec 11, 2023

dnkolegov Dec 11, 2023

raulk Dec 18, 2023

snissn Dec 18, 2023

raulk Dec 18, 2023

aakoshh Dec 19, 2023

snissn commented Dec 16, 2023

adlrocha Dec 18, 2023

raulk Dec 18, 2023

raulk Dec 18, 2023

raulk Dec 18, 2023

adlrocha Dec 18, 2023

raulk Dec 18, 2023

adlrocha Dec 18, 2023

raulk Dec 18, 2023

raulk Dec 18, 2023

adlrocha left a comment

raulk Dec 18, 2023

raulk Dec 18, 2023

raulk Dec 18, 2023

raulk commented Dec 18, 2023

raulk commented Dec 19, 2023

raulk commented Dec 19, 2023

ERC20: enable a subnet's supply to be determined by an ERC20 token in the parent. #313

ERC20: enable a subnet's supply to be determined by an ERC20 token in the parent. #313

Conversation

raulk commented Dec 10, 2023 • edited Loading

Context

Contribution

Non-goals

Design doc

TODO

github-actions bot commented Dec 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snissn commented Dec 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adlrocha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk commented Dec 18, 2023

raulk commented Dec 19, 2023

raulk commented Dec 19, 2023

raulk commented Dec 10, 2023 •

edited

Loading