chainsafe/chainbridge-solidity

⚙️ Solidity contracts for Sygma (ChainBridge)

chainbridge-solidity

ChainBridge uses Solidity smart contracts to enable transfers to and from EVM compatible chains. These contracts consist of a core bridge contract (Bridge.sol) and a set of handler contracts (ERC20Handler.sol, ERC721Handler.sol, and GenericHandler.sol). The bridge contract is responsible for initiating, voting on, and executing proposed transfers. The handlers are used by the bridge contract to interact with other existing contracts.

Read more here.

A CLI to deploy and interact with these contracts can be found here.

Dependencies

Requires nodejs and npm.

Commands

make install-deps: Installs truffle and ganache globally, fetches local dependencies. Also installs abigen from go-ethereum.

make bindings: Creates go bindings in ./build/bindings/go

PORT=<port> SILENT=<bool> make start-ganache: Starts a ganache instance, default PORT=8545 SILENT=false

QUIET=<bool> make start-geth: Starts a geth instance with test keys

PORT=<port> make deploy: Deploys all contract instances, default PORT=8545

make test: Runs truffle tests.

make compile: Compile contracts.

ChainSafe Security Policy

Reporting a Security Bug

We take all security issues seriously, if you believe you have found a security issue within a ChainSafe project please notify us immediately. If an issue is confirmed, we will take all necessary precautions to ensure a statement and patch release is made in a timely manner.

Please email us a description of the flaw and any related information (e.g. reproduction steps, version) to security at chainsafe dot io.

chainbridge-solidity's Issues

Combine create and vote

To remove the need for a relayer to check state to decide whether to create a new proposal or vote for an existing one, creation and voting should be combined into a single action. This also allows multiple relayers to submit creation transactions in a single block, and all the transactions can succeed.

This might be as simple as calling voteDepositPropsoal from createDepositProposal, although some checks can be removed and voteDepositPropsoal can be removed from the external ABI.

Sync Handlers

We need to ensure the handlers are consistent. Specifically, any changes to handling of addresses or token Ids need to be reflected in the other handlers.

Please update travis to run the CLI commands for NFTs once implemented.

cc @spacesailor24 @steviezhang

Update generic handler/centrifuge asset cli

We need to update the CLI to support the changes to the generic handler, and move the centrifuge command st othe new contract.

Optional whitelist only

Some instances may want to only allow interaction with a certain set of tokenIds/contracts. We should provde an option to enable/disable this (ie. not allow contract creation).

Relates to #38.

Burn list CLI command

We need CLI commands to add contracts to the burn lists

Update contracts to OpenZeppelin's latest

For example ERC20 should probably use this

And ERC721 should probably use this

@ansermino Brought a good point that all contracts were manually updated to Solidity version: 0.6.4 and it could be the cause of this

Solving who's token problem

Problem

If a token goes from Chain A to Chain B, and then back to Chain A. How does Chain A know that the token originally came from Chain A?

Solution

We implement a new string field tokenID. tokenID is composed of two pieces of data: the chainID & a string that is uniquely generated on that chain. For the smart contracts, the unique string will be the token address (it is unique enough). Therefore if we had a chain with chainID = 0, and a token address of "0x123...4", the tokenID would be 010x123....4. Similarly if the same chain had a chianID of 34, the tokenID would be 340x123...4.

Note: I used a string, this could very well be a byte array, for simplicity right now we will stick to a string.

How it works

A user creates a deposits

When a user makes a deposit, the handler will do a lookup to see if the token address has a token ID. If it has, not, then it will generate the tokenID accordingly and store a reference to it. If it has already seen the id, it takes custody of the funds, creates a depositRecord (see modified deposit record below).
Note: At this point we could do a check on the first two characters to see if they do not match our current chain (i.e its a synthetic), and we could burn the tokens. For now we will just take custody of them.

A relayer executes a deposit

In the handler, do a lookup on the tokenId, if there is an address associated to it. If there isn't, we create a new token contract (it clearly isn't from this chain), update the mapping and mint tokens. If it does already exist, we check the first two characters, if they match our chainID, then we transfer the tokens from within our safe balances. If they do not match, it is a synthetic, and we must call mint() on the specified token contract.

Proposed Changes

DepositRecord

Old

    struct DepositRecord {
        address _originChainTokenAddress;
        uint    _destinationChainID;
        address _destinationChainHandlerAddress;
        address _destinationChainTokenAddress;
        address _destinationRecipientAddress;
        address _depositer;
        uint    _amount;
    }

New

    struct DepositRecord {
        address _originChainTokenAddress;
        uint    _destinationChainID;
        id      _tokenID;
        address _destinationRecipientAddress;
        address _depositer;
        uint    _amount;
    }

ERC20 Handler

Note im recycling some old code found here

contract ERC20Handler is IDepositHandler, ERC20Safe {
    address public _bridgeAddress;

    struct DepositRecord {
        address _originChainTokenAddress;
        uint    _destinationChainID;
        string  _tokenID;
        address _destinationRecipientAddress;
        address _depositer;
        uint    _amount;
    }

    // DepositID => Deposit Record
    mapping (uint256 => DepositRecord) public _depositRecords;

    // Bi-directional maps
    // tokenID => token contract
    mapping (string => address) public _away; // change name
    // token contract => tokenID
    mapping (address => string) public _home; // change name

    modifier _onlyBridge() {
        require(msg.sender == _bridgeAddress, "sender must be bridge contract");
        _;
    }

    constructor(address bridgeAddress) public {
        _bridgeAddress = bridgeAddress;
    }

    function getDepositRecord(uint256 depositID) public view returns (DepositRecord memory) {
        return _depositRecords[depositID];
    }

    function deposit(
        uint256 destinationChainID,
        uint256 depositNonce,
        address depositer,
        bytes memory data
    ) public override _onlyBridge {
        address tokenAddress;
        address destinationRecipientAddress;
        uint256 amount;

        assembly {
            tokenAddress                   := mload(add(data, 0x20))
            destinationRecipientAddress    := mload(add(data, 0x40))
            amount                         := mload(add(data, 0x60))
        }

        string tokenID =_home[tokenAddress];
        if (tokenID == "") {
            // The bridge has never interacted with this token address before.
            // Therefore it must be native to this chain.
            
            // TODO need to get chainID from bridge
            tokenID = generateTokenStringID(chainID, tokenAddress);
            _home[tokenAddress] = tokenID;
            _away[tokenID] = tokenAddress;
        }

        lockERC20(tokenAddress, depositer, address(this), amount);

        _depositRecords[depositNonce] = DepositRecord(
            tokenAddress,
            destinationChainID,
            destinationRecipientAddress,
            depositer,
            amount
        );
    }

    // TODO If any address can call this, anyone can mint tokens
    function executeDeposit(bytes memory data) public override _onlyBridge {
        address tokenID;
        address recipient;
        uint256 amount;

        assembly {
            tokenID    := mload(add(data, 0x20))
            recipient  := mload(add(data, 0x40))
            amount     := mload(add(data, 0x60))
        }

        if (_away[tokenID] != address(0)) {
            // token exists
            stringChainID = tokenID[:1];
            uintChainID = StringToUint(stringChainID);

            // TODO need to get chainID from bridge
            if (uintChainID == chainID) {
                // this token is from our chain
                IERC20 erc20 = IERC20(tokenAddress);
                erc20.transferFrom(address(this), recipient, amount);
            
            } else {
                // this token is not from this chain
                ERC20Mintable erc20 = ERC20Mintable(tokenAddress);
                erc20.mint(recipient, amount);
            
            }
        } else {
            // Token doesn't exist
            ERC20Mintable token = new ERC20Mintable();

            // Create a relationship between the originAddress and the synthetic
            _away[tokenID] = address(token);
            _home[address(token)] = tokenID;

            // Mint new tokens
            token.mint(_to, _value);
        }
    }
}

tokenID is not correctly parsed in executeDeposit in ERC20Handler.sol

I have a test I'm working on that's verifying a depositer can deposit an ERC20 token and the recipient address can receive it

Everything is happening on the same chanID, so executing the deposit should just be a transfer from the ERC20 handler to the recipient i.e. the flow should be: depositer -> ERC20Handler -> recipient

However, executing the above test fails when asserting that the recipientBalance equals the depositAmount, and it seems to be because this if statement is falsey, causing the handler to create a new ERC20 contract and mint the tokens for the recipient there

Besides the test failing, my reasons to believe this is happening are:

1. I created a simple event that get's emitted inside the else clause, and when running the test it does get emitted
2. The following event gets emitted:

ERC20Mintable.Transfer(
  from: <indexed> 0x0000000000000000000000000000000000000000 (type: address),
  to: <indexed> 0xC5fdf4076b8F3A5357c5E395ab970B5B54098Fef (type: address),
  value: 10 (type: uint256)
)

where 0xC5fdf4076b8F3A5357c5E395ab970B5B54098Fef is the recipient address, and the zero address is the created contract minting tokens

I've verified that the tokenID is correctly created in the deposit method by calling both _tokenIDToTokenContractAddress and _tokenContractAddressToTokenID and comparing the results with the created tokenID in the test and the ERC20 address

So from my understanding, the only thing that could be going awry if that the tokenID is not correctly parsed in executeDeposit

Don't assume we will have a valid originChainTokenAddress when calling deposit

in all handlers we make ethereum-centric assumptions, replace instances of originChainTokenAddress with resourceID and only allow resourceIDs attached to whitelisted tokenAddresses to be used at this point of time

Update generic handler

We want to update the generic handler and the centrifugeAssetHandler.

centrifugeAssetHandler should become centrifugeAssetStore, removing the need for any bridge accounting/whitelisting.

The generic handler should require an interface execute(address who, bytes data) (similar to transfer(to, amount) for erc20). This is then implemented by centrifugeAssetStore so it can be called once the contract/id is whitelisted. Right now the who` has no effect in the asset store, but seems reasonable to include to make future additions simpler (lets be honest, most operations on-chain involve an address).

We should update the CLI to deploy these correctly and ensure all the existing centrifuge commands still work.

add code coverage tool

ChainId should be uint8

The definition of a chainId is a uint8 (https://github.com/ChainSafe/chainbridge-spec/issues/14). This should be reflected everywhere to avoid conversion issues (gas difference may be negligible?).

Choose executor on finalization

Currently, all relayers will attempt to execute a finalized proposal. We could instead select the last voter to be the executor, and include their address in the finalization event. Then a relayer could skip any finalization events where they are not selected.

cc @GregTheGreek @spacesailor24 @steviezhang

Bridge contract returning a list of Validators that voted on a particular proposal

If we’re not including staking, or aren’t worried about it right now, is there a reason we’d need a list of validators that voted on a particular proposal, or is just checking if a validator has voted enough for now?

i dont think we would?
- @GregTheGreek

Refactor DepositHandlers and Bridge contracts to make use of deposit types

Currently, there is no way for the Go event handlers to know the type of deposit made in the refactored deposit method in the Bridge contract

To fix this, @GregTheGreek, @steviezhang, and I decided to add a list of supported deposit types in the Bridge contract and having the deposit handlers adhere to one of the supported types. This means the deposit events will include the type of deposit that was made, allowing the Go code to handle each deposit event correctly

Additionally, the Bridge contract will have a new createSupportedTypeProposal that can be called by anyone, but can only be voted on by Relayers (Validators), and allows a new supported deposit type to be added

The list of supported types could look like:

uint256 public numberOfSupportedTypes = 0;

struct SupportedType {
    string _type;
    bool   _supported;
}

// supportedTypeID => SupportedType
mapping(uint256 => SupportedType) public _supportedTypes;

Where numberOfSupportedTypes triples as:

1. The total number of supported types
2. How supportedTypeIDs are created (supportedTypeID = ++numerOfSupportedTypes)
3. A way to get a list of all supported types
   • e.g. A getter method will have a for loop that iterates over _supportedTypes, returning an array of _supportedTypes[iterationCounter]._types

---

Initial supported types:

• ERC20
• ERC721
• GENRIC

@GregTheGreek should the native token be called: NATIVE or?

Differences in reverts on geth and ganache

There are issues testing reverts caused by Geth responding differently to ganache (ethereum/go-ethereum#19027)

It may be possible to check the raw response from the node inside of truffle, to confirm a revert. Further investigation should be done into how we can test reverts.

ERC721handler e2e testing

we need e2e testing for the 721 handler in order to make sure it functions as desired

Change depositNonce to uint64

This will be reflected in the spec. Probably not urgent as we currently use uint256.

Improve overview docs

Some ideas for things to include:

• What type of Erc20/721 setups we support (and how do they work)
• How does a handler function? What can a generic handler do?
• We use addresses and chain IDs to compose resource IDs, any reason for this?
• How is the bridge administered?
  ...

This could be part of the readme of live in its own page on notion.

Smart contract tests

These are the remaining tests to be written after the contract refactor:

As an addition to the below, there also needs to be test to verify the flows of deposits:

• ERC20 deposit flow

• ERC721 deposit flow

• Generic deposit flow

• Bridge.sol

  • executeDepositProposal
  • createValidatorThresholdProposal
  • voteValidatorThresholdProposal
  • getCurrentValidatorThresholdProposal
  • getValidatorThreshold

• ERC20Handler.sol

  • executeDeposit
  • withdrawERC20

• ERC721Handler.sol

  • verify extra data is retrieved as expected from assembly block (in executeDeposit)
  • executeDeposit
  • withdrawERC721

Replace tokenId with resourceId

Usage of the name tokenId should be replaced with resourceId, except when referring to nft tokenIds

Should executeDeposit have _onlyBridge modifier

Specifically, this line:

function executeDeposit(bytes memory data) public override {
        address destinationChainTokenAddress;
        address destinationRecipientAddress;
        uint256 amount;

        assembly {
            destinationChainTokenAddress := mload(add(data, 0x20))
            destinationRecipientAddress  := mload(add(data, 0x40))
            amount                       := mload(add(data, 0x60))
        }

        ERC20Mintable erc20 = ERC20Mintable(destinationChainTokenAddress);
        erc20.mint(destinationRecipientAddress, amount);
    }

Previously, @GregTheGreek and I had discussed that executeDeposit should be callable by any address, but I think it makes sense that the executeDeposit methods within the handlers get the _onlyBridge modifier and Bridge.sol's executeDepositProposal stays callable by any address

Make Handler Withdraw Methods Callable

Proxy contract

For upgrading reasons, we should probably have a proxy contract, that sits in-front of the bridge/validator contract. Are there any design considerations we should account for?

@noot @steviezhang @spacesailor24

Should the Deposit event emitted contain a field indicating the handler type?

Noticed an issue on ChainSafe/ChainBridge#284 where, because the event being emitted after deposit is called in the bridge reroutes to the intended handler, when subscribed to the event, it's hard to differentiate between depositing an ERC20, ERC721, or another token.

This has not become an issue yet because we have not written tests involving ERC721 as of yet.

If we add the field, the two events could be condensed to a single DepositedTokenSignature and use the value of the handler type field to determine which unpacking function to use.

Additional CLI functionality

• Query centrifuge handler hash, new command
• Specify the contract address as an optional param for interactions, default to constants
• Specify host in addition to port

Allow multiple geth instances

We need to update our geth scripts to allow for multiple instances for e2e tests in ChainBridge.

Burnlist

We need to be able to burn tokens that are deposited, as a feature request. This is @GregTheGreek 's proposed solution:

mapping(address => bool) public burnMap;

func deposit(tokenAddress) {
  ...
  if (burnMap[tokenAddress) {
    // send to 0x0
  }
}

We basically just need an additional whitelist and admin call to track them, and then modify the recipient in the transferFrom.

Optimize test deployment

I think there is an easy way to get our tests against geth to run faster.

Currently we await each deployment, forcing them to be in seperate blocks. Some contracts depend on others, but if we simply seperate them into two container promises they should deploy in fewer blocks.

Something this like this:

chainbridge-solidity/test/chainBridge/depositERC20.js

Lines 33 to 38 in 9efda44

	RelayerInstance = await RelayerContract.new([], relayerThreshold);
	BridgeInstance = await BridgeContract.new(originChainID, RelayerInstance.address, relayerThreshold);
	OriginERC20MintableInstance = await ERC20MintableContract.new();
	OriginERC20HandlerInstance = await ERC20HandlerContract.new(BridgeInstance.address);
	DestinationERC20MintableInstance = await ERC20MintableContract.new();
	DestinationERC20HandlerInstance = await ERC20HandlerContract.new(BridgeInstance.address);

Could be modified to have as many of these grouping as necessary:

await Promise.all([
    RelayerContract.new([], relayerThreshold).then((instance) => BridgeInstance = instance),
    ERC20MintableContract.new([], relayerThreshold).then((instance) => OriginERC20MintableInstance = instance),
    ERC20MintableContract.new([], relayerThreshold).then((instance) => DestinationERC20MintableInstance = instance)
])

Centrifuge Asset not checked if already been stored when deposited

While checking if an asset is stored is trivial, the complication comes from how to deal with it within the GenericHandler.sol here. The call could be made to the _assetsStored mapping within CentrifugeAsset.sol and presumably a return value would be received, but how would you be able to make the comparison between what was received and what was expected in a generic manner that would work with any contract.

Update ERC20Handler to accept an array of tokenIDs upon deployment

ERC20Handler.sol needs to accept an array of tokenIDs in the constructor, or else you can't use an existing destination token contract, it'll always deploy a new token contract when executing a deposit

Use proposals for administration

Currently, we have some proposals for modifying the relayers/threshold. We need to provide administration for resourceIds as well. Perhaps it makes sense to build these into a separate handler, such that the existing proposal structure can be used.

Following the idea of a generic handler, we could implement an execute function that interacts with the relayer contract and possibly the other handlers. It may be helpful to think of these operations as CRUD on a single type (relayer => address, threshold => uint32, resourceId => resourceId)

Eg. Add a new relayer

The admin handler would have special privileges to call methods on the relayer contract. One such method would be admin(bytes data). data consists of action (byte) + relayer (address), where action == 0 adds a relayer, action ==1 removes a relayer.

Eg. Register a resource ID

The resource registry exists as part of a handler or a separate contract. The contract would provide the method admin(bytes data), where data is operation (byte) + resourceId (bytes). operation == 0 would create an entry for the new resourceId, operation == 1 would update, operation == 2 would delete.

Single relayer setup not compatible with bridge logic

Presently the DepositProposalVote only gets emitted when an actual vote is placed, so the initial vote does not produce an event. This affects the bridge logic, only when the relayer threshold is 1, as we depend on the DepositProposalVote event to trgger executeDeposit.

Force minting for burnables

It seems logical that any token we mint, we should burn in the inverse. Right now we use chain ID from the resource ID to decide whether to mint or transfer, it would be more flexible if we abandoned this and just minted for all token on the burn list (otherwise we use transfer).

Correctly parse metadata on deposit of erc721

We are currently providing the metadata for the token as a parameter, we should rather use _tokenURI() on the erc721 contract. This may or may not exist on the contract, if it does not we can simply use an empty metadata value.

To verify if a contract supports metadata, we need to query the interface ID (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/b7452960bed44db4bbd0da2101af37c7525d8369/contracts/introspection/ERC165.sol#L33). The interface ID we are looking for is 0x5b5e139f(source)

Relayer.sol needs to be updated to match Bridge.sol

Specifically, around how votes are tracked - Relayer.sol was never updated with replacing tracking votes as uints to address[]

Needed changes:

• replace number of votes with array of voter addresses for RelayerProposal and RelayerThresholdProposal
• Combine createRelayerProposal and voteRelayerProposal

Add tests for variable sized payloads

We should extend testing on the assembly data deconstruction considerably. Specifically, we should test varying payloads with different address sizes for recipient and different metadata sizes for erc721.

Internal audit

Similar to ChainBridge, we should take a step back and do a brief audit of our work so far.

We should look at:

• cleanup tasks (unused code, duplicated code, outdated comments)
• structure/contract layout
• over-engineering of components (things were easier than anticipated, perhaps)
• etc.

This should result in documented summary of the findings, from which we can then extract any necessary tasks to be completed.

(Here's what I came up with for ChainBridge: ChainSafe/ChainBridge#305 (comment))

Rename variables

The following variables can probably all just drop the origin prefix

• https://github.com/ChainSafe/chainbridge-solidity/blob/master/contracts/handlers/ERC721Handler.sol#L75
• • https://github.com/ChainSafe/chainbridge-solidity/blob/master/contracts/Bridge.sol#L108
• • https://github.com/ChainSafe/chainbridge-solidity/blob/master/contracts/handlers/ERC20Handler.sol#L181

• Could these be pulled out into "handler helpers"?

Deposit and ExecuteDeposit in the GenericHandler Make External Call Differently

deposit:

if (_contractAddressToDepositFunctionSignature[contractAddress] != bytes4(0)) {
            (bool success,) = contractAddress.call(metadata);
            require(success, "delegatecall to contractAddress failed");
        }

executeDeposit:

bytes4 sig = _contractAddressToExecuteFunctionSignature[contractAddress];
        if (sig != bytes4(0)) {
            bytes memory callData = abi.encodePacked(sig, metaData);
            (bool success,) = contractAddress.call(callData);
            require(success, "delegatecall to contractAddress failed");
        }

deposit should look like executeDeposit

Remove Unnecessary Getter Method from Smart Contracts

There are several getter methods in the contracts that don't need to be there since various variables/mappings are public - this would reduce gas costs

Enable admin controls

We need to provide admin control to:

• whitelist token IDs
• add/remove relayers
• change relayer threshold
• transfer owned tokens (to retrieve deposits)
• enable emergency halt of Bridge
• additional admin call for burnlist proposed in #68

Contrary to what I previously stated, we should keep existing voting functionality and instead add this as an alternative. For the controls that don't yet exist we can have this as the only option.

A simple modifier should be sufficient for now (eg. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol).

We should include a control on the bridge to disable deposits/transfer by the admin.

721 CLI support

We need to be able to interact with erc721 contracts, much like we can with erc20s already (eg. mint). We should be able to initiate a transfer and verify the ownership of a specific token.

Remove originChainHandlerAddress from _depositProposals mapping

Remove from this mapping to make more sense for interacting with Centrifuge

Extend CLI functionality

Presently our CLI handles only a few interactions and has limited configuration.

We should add additional functionality including:

• Ability to use a private key for interactions

deploy

• Deploy erc721 & handler
• Add erc20, erc20Handler, erc721, erc721Handler, centrifugeHandler flags to indicate which contracts to deploy

mint

• Flag to mint erc721
• Add recipient option to specify the to field

transfer

• Execute ERC721 transfer (incl. minting)

erc20Balance

• checks balance for address at some erc20 contract address

Remove RuntimeBytecode variable From Bindings

We currently aren't using the RuntimeBytecode field, and it is causing issues for the Go code when it comes to checking the binding on the CI.

We should both remove the code that is adding the variable, and the code that is deriving the variable.

Add fee to deposits

The deposits should have an optional fee (settable in the constructor & a admin function).

The fee should be a fixed amount of ETH per deposit.

keccak hash should include handler address

The handler address passed into executeDeposit() should be the hash of the metadata + destinationHandlerAddress.

The current setup allows a relayer to chose the handler they want, without taking the vote into account.

ERC721Handler needs optional whitelisting

ERC721Handler.sol needs to mimic ERC20Handler.sol in terms of optional whitelisting functionality

Function comments

Many of the functions within the contracts are missing doc comments.