SNO-612: Extend sendToken for Polkadot-native assets #927

doubledup · 2023-08-16T18:05:23Z

Stacking this on #926. I'm not sure about initiating burn within Assets, given the need to invoke an agent. Would it be more appropriate to burn in Gateway? (where we invoke agents & register Polkadot tokens) Should we rather move Polkadot token registration into Assets as well?

SNO-612

codecov · 2023-08-16T18:07:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (polkadot-assets-on-ethereum@4ba3e14). Click here to learn what that means.

❗ Current head b7e3511 differs from pull request most recent head 9a216bb. Consider uploading reports for the commit 9a216bb to get more accurate results

Additional details and impacted files

@@                      Coverage Diff                       @@
##             polkadot-assets-on-ethereum     #927   +/-   ##
==============================================================
  Coverage                               ?   72.43%           
==============================================================
  Files                                  ?       41           
  Lines                                  ?     1810           
  Branches                               ?       92           
==============================================================
  Hits                                   ?     1311           
  Misses                                 ?      473           
  Partials                               ?       26

Flag	Coverage Δ
solidity	`60.62% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vgeddes · 2023-08-16T18:55:29Z

contracts/src/Gateway.sol

+    struct RegisterTokenParams {
+        /// @dev The ID of the agent to register the token for
+        bytes32 agentID;
+        /// @dev The name of the new token
+        string name;
+        /// @dev The short symbol of the new token
+        string symbol;
+        /// @dev The number of decimal places allowed
+        uint8 decimals;
+    }
+
+    // @dev Register a new fungible Polkadot token for an agent
+    function registerToken(bytes calldata data) external onlySelf {
+        CoreStorage.Layout storage $ = CoreStorage.layout();
+
+        RegisterTokenParams memory params = abi.decode(data, (RegisterTokenParams));
+
+        address agent = $.agents[params.agentID];
+        if (agent == address(0)) {
+            revert AgentDoesNotExist();
+        }
+
+        bytes memory call = abi.encodeCall(AgentExecutor.registerToken, (params.name, params.symbol, params.decimals));
+        bytes memory response = _invokeOnAgent(agent, call);
+
+        address token = address(bytes20(response));
+        $.ownerAgentIDs[token] = params.agentID;
+    }


The gateway already has an Command.AgentExecute command for sending arbitrary messages to Agents. Lets use that rather than adding special-purpose commands for higher-level application stuff.

I understand we do need to keep a token registry in the gateway. So you could make the agent call back into the gateway to update the registry.

Oops, my mistake, your approach here is correct. My memory was foggy as I just got back from leave.

Actually, yes, we should be reusing using 'Command.AgentExecute' instead of creating a new toplevel 'Command.RegisterToken'.

The call back into the gateway opens a door to attackers: they can change the agentID for any token. This won't let them control the token (as they're not the owner recorded in the token contract), but it can be used to grief legitimate owners by changing the token's agentID so that calls to the token fail.

So we'll need some kind of authorisation when updating the token registry. It might be enough to require that the token hasn't been registered before.

contracts/src/Assets.sol

contracts/src/storage/CoreStorage.sol

vgeddes · 2023-08-17T08:36:58Z

contracts/src/Gateway.sol

@@ -410,6 +415,35 @@ contract Gateway is IGateway, IInitializable {
        _transferNativeFromAgent(agent, payable(params.recipient), params.amount);
    }

+    struct RegisterTokenParams {


When registering a foreign token, these params also need to include a unique and stable ID for the original asset on the Polkadot side.

This ID will be used later on for Polkadot->Ethereum transfers, to lookup the address of the erc20 contract of the wrapped token.

On BridgeHub, I suggest reusing the code below to produce this ID.

snowbridge/parachain/pallets/control/src/lib.rs

Line 145 in 60a82e0

let agent_id = T::AgentHashedDescription::convert_location(&location)

The (name, symbol, decimals) metadata can't be used for this purpose. That's just metadata and has no guarantee of uniqueness.

Agree! Arbitrary transact also depends on the mapping here(asset_id <-> location) should be a runtime storage somewhere to support dynamic fee, currently we only support DOT to be paid as the fee of BuyExecution.

vgeddes · 2023-08-22T17:38:08Z

contracts/src/Gateway.sol

@@ -412,13 +412,14 @@ contract Gateway is IGateway, IInitializable {
    }

    // @dev Register a new fungible Polkadot token for an agent
-    function setTokenOwnerAgentID(address token, bytes32 agentID) external {
+    function registerToken(bytes32 tokenID, address token, bytes32 agentID) external {


I've thought of a way we can prevent malicious parachains from using Transact to call back into gateway via an agent.

In AgentExecutor.sol, we make sure the destination of the transact is not the gateway contract. If it the destination is the gateway, we disallow the transact.

What we still need for this registerToken is a guard to ensure it's only being called by a registered agent. We may need to introduce another storage field which allows to perform this lookup. Unfortunately the storage field below only allows us to lookup by agentID:

snowbridge/contracts/src/storage/CoreStorage.sol

Line 16 in 8b8f1b5

mapping(bytes32 agentID => address) agents;

We receive the relevant agentID as the 3rd parameter here, so we can check that msg.sender == agents[agentID]. This means that only a registered agent can call registerToken and that they must register tokens for themselves. It's slightly stricter than just "registered agents only", but seems reasonable as the intention is for agents to register their own tokens anyway.

Move registerToken to gateway command so that the return value can be set in the gateway's storage.

Register, mint, then transfer.

claravanstaden · 2024-02-20T10:21:41Z

Closing, will reopen if it becomes relevant.

doubledup requested review from vgeddes, alistair-singh and yrong August 16, 2023 18:05

doubledup self-assigned this Aug 16, 2023

vgeddes reviewed Aug 16, 2023

View reviewed changes

yrong reviewed Aug 17, 2023

View reviewed changes

contracts/src/Assets.sol Outdated Show resolved Hide resolved

alistair-singh reviewed Aug 17, 2023

View reviewed changes

contracts/src/Assets.sol Outdated Show resolved Hide resolved

contracts/src/storage/CoreStorage.sol Show resolved Hide resolved

doubledup added the do-not-merge label Aug 17, 2023

vgeddes reviewed Aug 18, 2023

View reviewed changes

doubledup force-pushed the david/sno-612 branch 2 times, most recently from f6fdd41 to b340c95 Compare August 21, 2023 20:47

doubledup force-pushed the david/sno-611 branch from ebe5178 to 48f4717 Compare August 22, 2023 13:12

vgeddes reviewed Aug 22, 2023

View reviewed changes

doubledup mentioned this pull request Aug 23, 2023

SNO-608: Add RegisterToken & MintToken commands #931

Closed

Base automatically changed from david/sno-611 to polkadot-assets-on-ethereum August 24, 2023 15:41

doubledup removed the do-not-merge label Aug 24, 2023

yrong mentioned this pull request Aug 25, 2023

Re-Entrancy tests #933

Closed

doubledup added 12 commits August 28, 2023 11:28

Sort handler dispatch by order of definition

6640871

Sort commands by order of handler definition

64b8122

Add token registry

c33657f

Move registerToken to gateway command so that the return value can be set in the gateway's storage.

Remove unused imports

8ba41a5

Extend sendToken for registered Polkadot assets

a0d0dbf

Fix docstrings

4d15668

Burn tokens from sender, not the token itself

85352b1

Move registerToken into agentExecute

33f2a63

Add notes about token movement in sendToken

ed8f149

Add tokenID mapping

2932fa7

Mint tokens by tokenID

3173fe1

Sort AgentExecuteCommand chronologically

9a216bb

Register, mint, then transfer.

doubledup force-pushed the david/sno-612 branch from b7e3511 to 9a216bb Compare August 28, 2023 15:56

yrong added the do-not-merge label Jan 24, 2024

claravanstaden closed this Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNO-612: Extend sendToken for Polkadot-native assets #927

SNO-612: Extend sendToken for Polkadot-native assets #927

doubledup commented Aug 16, 2023

codecov bot commented Aug 16, 2023 •

edited

Loading

vgeddes Aug 16, 2023

vgeddes Aug 16, 2023

vgeddes Aug 18, 2023

doubledup Aug 21, 2023

vgeddes Aug 17, 2023

yrong Aug 21, 2023 •

edited

Loading

vgeddes Aug 22, 2023

doubledup Aug 22, 2023

claravanstaden commented Feb 20, 2024

SNO-612: Extend sendToken for Polkadot-native assets #927

SNO-612: Extend sendToken for Polkadot-native assets #927

Conversation

doubledup commented Aug 16, 2023

codecov bot commented Aug 16, 2023 • edited Loading

Codecov Report

vgeddes Aug 16, 2023

Choose a reason for hiding this comment

vgeddes Aug 16, 2023

Choose a reason for hiding this comment

vgeddes Aug 18, 2023

Choose a reason for hiding this comment

doubledup Aug 21, 2023

Choose a reason for hiding this comment

vgeddes Aug 17, 2023

Choose a reason for hiding this comment

yrong Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

vgeddes Aug 22, 2023

Choose a reason for hiding this comment

doubledup Aug 22, 2023

Choose a reason for hiding this comment

claravanstaden commented Feb 20, 2024

codecov bot commented Aug 16, 2023 •

edited

Loading

yrong Aug 21, 2023 •

edited

Loading