GMP in the provided tx fails with incorrect depositor in deposit data

Expected Behavior

GMP passes succesfully

Current Behavior

GMP fails with the above mentioned error message

Possible Solution

Wrong execution data is encoded

Steps to Reproduce (for bugs)

deposit function locks or burns the ERC1155 token, but it does not confirm if the token address is correct.
The interfaceID was defined already in the contract, and it can be used for checking the token
address.

Implementation details

Check the token address using interfaceID.

Testing details

unit tests

Acceptance Criteria

passing unit test with interfaceID checks

As we want our generic handler to be compatible with a wide array of chains, the format of cross-chain data that we are bridging should be chain agnostic. Currently, we are presuming information like address length or function signature.

Implementation details

• All address lengths should be dynamic, where you first define address length trailed by the address itself.

# example
addressLength bytes32 bytes 0  - 32
address       bytes   bytes 32 - (32 + addressLength)

• Try to abstract the function signature to something like functionIdentifier with the same dynamic properties as described for addresses.

Testing details

• Expand current unit tests based on made changes

Acceptance Criteria

• Have a team consensus on the final design of cross-chain data
• All unit tests passing

deposit function defines a named return variable. However, it is neither used nor updated, and the function
returns empty bytes.

Implementation details

Remove the unnecessary name from the return variable.

Testing details

n/a

Acceptance Criteria

no compiler warnings

Current support for deploying different fee handlers trough migration scripts is limited. This could be especially become a hurtle on deploying new networks in production like environments (with a need for deploying and configuring multiple fee handlers on initial deployment).

Implementation details

To enable deployment of multiple fee handlers and using them when registering on fee router, requires an significant refactor of migration config file format. Below is one possible approach to new format that would enable us to:

• define multiple fee handler instances that need to be deployed
• map different fee handlers for each resource (per destination domain)

{
    "fee": {
        "basic": ["0.1", "0.2"],
        "oracle": [
            {
                "target": "evm",
                "gmp": true,
                "params": {
                    "gasUsed": "",
                    "feePercentage": 0
                },
                "address": ""
            }
        ]
    },
    "erc721": [
        {
            "name": "ERC721",
            "fees": [
                {
                    "destDomainID": "5",
                    "feeHandler": "oracle-0"
                },
                {
                    "destDomainID": "2",
                    "feeHandler": "basic-1"
                }
            ]
        }
    ]
}

Testing details

• Manually test executing initial migration with multiple fee handlers defined, and different fee handlers registered for same resource.

Acceptance Criteria

• Migration scripts have full support for deploying and setting fee handlers

owner storage variable is set in a constructor but is not intended to be changed later.

Implementation details

set owner variable as immutable

Testing details

n/a

Acceptance Criteria

set owner variable as immutable

Since we're storing _contractWhitelist, _burnList and other data with key contractAddress in different mappings, we can define a struct that will contain all this data and store it in a single mapping.

Implementation details

Define struct that will contain data about contract address _contractWhitelist, _burnList and other with contractAddress and store it in a single mapping.

Testing details

Unit and e2e tests

Acceptance Criteria

[ ] Current test passing with new logic.

With the current implementation XC20 token can be only minter to the issuer (XC20Handler), which is not the desired behaviour because bridged tokens can't be minted to the recipient on destination chain.

Implementation details

Override mint() function on XC20Handler to first mint tokens to the handler and then transfer them to the recipient.

Testing details

Unit, e2e tests

Acceptance Criteria

[ ] All current tests passing
[ ] Tests that check tokens are firstly minted to the handler then transferred to the recipient.

deposit function locks or burns the ERC1155 token, but it does not confirm if the token address is correct.
The interfaceID was defined already in the contract, and it can be used for checking the token
address.

Implementation details

Check the token address using interfaceID.

Testing details

unit tests

Acceptance Criteria

passing unit test with interfaceID checks

We need to maintain all the contract methods and their sigahshes so there is always one place where to check Access Control sighashes integrity and actuality.
Now the only place we have this list is
https://www.notion.so/chainsafe/Sygma-smart-contracts-deployment-guide-STALE-b1c88796db7a4edabd4dcb3940f3a9e1
and Migrations (was generally added there from Notion page above)

Implementation details

• Add file (json, yml, etc) that contains all protected by AccessControlSegregator methods Text Signatuer and Sig Hash (4 bytes of Hash)
• Think/implement about automating process of mainting this file (some GA script that parse .sol file and then generates this file for every release)
• Think how this could be used by migrations as well (used as AC config file for migrations)

Testing details

[] Manually check that script works as should

Acceptance Criteria

[] File create
[] automation script created

Our current DynamicGenericFeeHandlerEVM.sol implementation of dynamic fee handling for generic messages would not work properly if deployed.

Logic inside function for calculating fee fetches handler based on resourceID and assumes it is compatible IERCHandler interface. This will never be true, as this will always be PermissionlessGenericHandler - that doesn't have function _resourceIDToTokenContractAddress.

This is tight to a bigger problem of DynamicFeeHandler assuming that fee is being paid in token that is transferred - this is not applicable for generic messages. What we actually want is to collect fees in base (native) tokens when executing generic calls.

Implementation details

These are just suggestions and final solution requires deeper dive into solidity code:

• We need to add some logic inside DynamicFeeHandler that will decide if the fee is being paid in base (native) token or some erc20 token.
• In conjunction with this, the implementation of _calculateFee inside DynamicGenericFeeHandlerEVM.sol needs to be changed to return 0x00..00 address or something like this - this information will be used to decide DynamicFeeHandler as mentioned in the first point.

Testing details

Revisit current tests, as they were written with wrong presumptions, so tests did not catch this.

• Add unit tests

Acceptance Criteria

• Passing unit tests

We need to provide explicit information about gas costs of main bridge operations

Implementation details

Please refer to this doc to take it as a base tempalte
https://hackmd.io/jqLQHOAjTvi0Mvump8SgnQ
Make sure to mention executions bundling also

Testing details

Acceptance Criteria

[] Gas consumption that compares v1 v2 and v3 written

setFeeOracle and setFeeProperties functions update important property variables but do not emit corresponding events.

Implementation details

Emit appropriate events on updating these properties.

Testing details

unit tests

Acceptance Criteria

events are emitted in tests

In order to deploy all the smartcontracts to testnet environment we should write necessary migration scripts

Implementation details

Migrations scripts should behave according to the Deployment flow for TestNet
Finishing actions of Migrations scripts should be transferring all the admin rights to Admin Multisig
Networks:
Polygon, ETH (Goerli), Moonbeam (Testnets)
Use pre-deployed tokens from TestNet.

Do not deploy any permissionless Generic handler for now. Only Old permissioned handler.

List of access rights to grant https://www.notion.so/chainsafe/Sygma-Admin-Governance-In-Progress-370c78f5c362423295a2dd620fbd6bc8
As you can see there will be 2 multisig addresses.

Testing details

Acceptance Criteria

Things to do:

• prepate function calls from template that need to be executed on multisig
• update shared configuraton
• register new network on currently supported network handlers

Implementation details

• execute batched function calls on multisig
• manually register resources on currently supported networks

Testing details

• manually check configs and template to be executed on testnet
• manually test bridging between Sepolia

Acceptance Criteria

• successfully bridge tokens between currently supported network and Sepolia

As the gas limit for the destination chain will be defined as a parameter in the new message format, we need to update FeeHandlerWIthOracle to support parsing this parameter and calculating tx cost.

Implementation details

We will add a new param to oracle message (msgGasLimit) which will be parsed by FeeHandlerWithOracle and tx cost is calculated with that value - not like before, where we used a variable from the contract (that has a public setter).

Old formula for destination gas:

destTxCost = oracleDestGasPrice * contractGasLimit * oracleMessage.ter / 1e18

New formula for destination gas:

destTxCost = oracleDestGasPrice * messageGasLimit * oracleMessage.ber / 1e18

Testing details

• Add unit tests for new fee handler

Acceptance Criteria

• Add new fee handler
• Passing unit tests
• Passing e2e locally with new version of fee handler

https://docs.openzeppelin.com/contracts/4.x/api/token/erc721#ERC721Enumerable

Not it is a bit obscure for someone from outside the team to understand decimal conversion happens in smart contract. We should tech documentation for this in {repo}/wiki section.

Implementation details

Documentation should be targeting only developers, so it should be laconic and strict. Describe the reasoning, implementation, possible conrercases and problems.

Testing details

Acceptance Criteria

[] Documentation written and reviewed

It would be useful that we can return some data on executeProposal from handlers like we do right on on deposit, this data would then be emitted in the ProposalExecution, this would be useful for indexing events.

Explore the possibilities on this.

Implementation details

Handlers return data from executeProposal and pass it to ProposalExecution event

Testing details

unit and e2e tests

Acceptance Criteria

[ ] - handlers return data and pass it to the ProposalExecution event
[ ] - existing and new tests covering this implementation are passing

Story

• As Sygma Operators
• We want Sygma to support fees based on a % of the transferred token amount
• So that we can charge users relative to the transferred amount

Implementation details

Implement fee handler with dynamic % fee, based on token amount that is being bridged, not the USD value (current dynamic fee handler)

You can check out formal definition of this fee strategy inside technical docs of relayer.

For now we only need to support collecting fees in ERC20 tokens (not base currency).

Testing details

Unit tests

Acceptance criteria

[ ] Passing current unit and e2e tests
[ ] Passing unit tests that cover the new functionality

As per research, XC20Safe contract will contain the implementation for xc20 asset related methods

Implementation details

• Add IERC20Plus interface
• Implement mint, burn, lock, release method that implements IERC20Plus interface for xc20 asset.

Details can be found in the research doc

Testing details

• Add contract testcases

Acceptance Criteria

XC20Safe contract with mint, burn, lock, release method that implementing IERC20Plus interface
All related testcases pass

mintERC20 function ignores the return value from the transfer function of the ERC20 token. The transaction can succeed even if a token transfer fails. This could have downstream effects on the accounting of tokens by the bridge.

Implementation details

mintERC20 function should check return value of transfer.

Testing details

Minting tests with success and fail cases.

Acceptance Criteria

Unit and e2e passing

Create docker image with deployed contracts on each release from the migration scripts.

Since we want to get rid of Golang CLI inside relayers , we need to some tool do deploy smart contracts.
Since our smart contracts are highly upgradable it would be right to use Truffle migrations.
Main goal (for now) is to have development instance fully covered with Truffle migrations, so for example we can make them part of our CI/CD and deploy to our develop environment without delays or use them for local development.
In future it is planned to have this migrations also for production and testnet environments.

Implementation details

Implement Truffle migrations, please keep in mind different environments necessity. It would be nice to have some config file that provide all the necessary inforatmion (like resourceID's, domainIDs, adminAddress, etc. etc.) and based on this file this migration scripts should deploy the contracts.
Migration scripts should much develop instance

Testing details

Acceptance Criteria

[] Migrations developed

Story

As a bridge app developer
I want to have an adapter contract that invokes Bridge to be able to emit events with bridge nonce and other useful data
So that I can monitor events on the destination chain.

Background

Currently, the Bridge only emits events and doesn't return anything which is fine as long as dapp user interacts with the handler directly. If you use an adapter contract, the sender address in the Deposit event is always the address of the adapter contract which makes it really hard to find the Deposit event you need.

Details

Scenarios

Scenario:
Given I am <type of user/persona>
When
And
Then

Implementation details

GenericHandler deposit method should return (depositNonce, handlerResponse) to allow AdapterContract to emit event with Nonce and some other identifiable data.

Testing details

Acceptance criteria

After resolving #189,
there was some miscommunication and the Sygma adapter for Hashi was written for an older version of PGH that the latest. (address for the temp PGH: 0xF830A5822495fe4e4c1C98D28DD61A329B370200).
After the adapter gets updated we need to migrate back to the latest version of PGH (the current address saved in notion).

Substrate has a different fee calculation formula, this FeeHandlerWithOracle will calculate the substrate fee and convert it into erc20 amount using given params.

Implementation details

Fee oracle is responsible to provide final_fee to FeeHandler.

Example of how fee calcualted on substrate:

final_fee = base_fee + length_fee + [targeted_fee_adjustment * weight_fee] + ?tip?;

For ERC20 transfer ETH→ PHA FeeHandlerWithOracle on the ETH should have gasLimit set to 1. In this case this equation
txCost = oracleMessage.dstGasPrice * _gasUsed * oracleMessage.ter / 1e18; - original ETH <> ETH formula
equals to
txCost= final_fee * oracleMessage.ter / 1e18
What we have done is we made a statement that oracleMessage.dstGasPrice * _gasUsed == final_fee
And this final_fee will be provided by the FeeOracle

Testing details

contract js test

Acceptance Criteria

FeeHandlerWithOracle should be able to calculate fee following substrate fee instructions
FeeHandlerWithOracle should be able to convert calculated substrate fee into erc20 amount using given price rate

Use max fee field as gas for the external permissionless generic call.

Implementation details

Decode max fee from the execution data and set it to call (call{gas: maxFee}.functionCall())

Testing details

• unit tests that the call is not reverted even though gas limit is exceeded

Acceptance Criteria

The smart contracts currently use require and revert string messages for error handling. However,
using a revert with a custom Error, instead of a string error message, considerably reduces the smart
contract size and optimizes gas costs on deployment. Contracts:
Bridge -> _onlyAllowed
ERC20Safe -> _safeCall

Implementation details

Use a revert with a custom Error to handle errors.

Testing details

unit tests

Acceptance Criteria

existing unit tests passing with new implementation

Description:

The XC20Handler idea is the same as ERC20Handler but resourceID will be mapped between ERC20 on EVM chain and XC20 on Astar chain. When bridge sees the resource that associated with XC20, router the call to XC20Handler.

Implementation details
Add XC20Handler contract to specifically handle XC20 related operations such as Mint, Burn, Lock and Release on Astar chain.
The logic in the XC20Handler mostly remains the same as ERC20Handler. It should contain executeProposal method that handles the execute xc20 asset to Astar, and deposit method that handles xc20 asset deposit from Astar, withdraw method that handles manual release to Astar

Testing details
Add contract testcases

Acceptance Criteria
XC20Handler contract with executeProposal, deposit and withdraw method with proper logic that connecting with XC20Safe
All related testcases pass

We want to align with shared naming convection defined by technical documentation of fee estimate message.

Implementation details

Originally, when processing the fee estimate messages in the Substrate context, we used finalFee name for this property. When considering substrate naming and actual context of this property, it is clear that using name inclusionFee makes more sense. This number represents the amount of gas needed for the specified extrinsic to be included.

Acceptance Criteria

• rename property finalFee to inclusionFee

Right now if we want to have different fees for different routes we would need to deploy multiple basic/percentage fee handlers.

Implementation details

Introduce _domainResourceIDToFee mapping that will store different fees based on route.

Testing details

Acceptance Criteria

All existing tests are passing after update

Things to do:

• run migration scritps to deploy contract on Sepolia
• update shared configuraton
• register new network on currently supported network handlers

Implementation details

• run migration scripts
• manually register resources on currently supported networks

Testing details

• manually check configs
• manually test bridging between Sepolia

Acceptance Criteria

• successfully bridge tokens between currently supported network and Sepolia

Decimal difference is a potential issue, it could occur between evm and substrate chain, or between two evm chains, a proper handler is needed to take care such issue.

Implementation details

details could be found in the SoW research doc

Testing details

contract js test

Acceptance Criteria

• the new handler should allow source and dest decimal register with one token address
• the new handler should be able to calculate the final amount
  [] Add migrations

The referenced functions do not modify the state of a smart contract. View mutability can be used for
these functions so that state modification can be prevented, and mutability be made explicit.

Implementation details

Use appropriate mutability for functions.

Testing details

n/a

Acceptance Criteria

no compiler warnings

Currently, after the fee data is fetched from fee oracle the amount of tokens can always be set to 0 to pay the minimum (tx cost) amount of tokens instead of the percentage of the total amount.

Expected Behavior

It should check fee data amount against deposit data amount and fail it if is not the same.

Current Behavior

It accepts any value as amount.

Possible Solution

Deposit data is already sent to the collectFee and amount from deposit should be compared to fee data amount.

Steps to Reproduce (for bugs)

Versions

ChainBridge commit (or docker tag):
chainbridge-solidity version:
chainbridge-substrate version:
Go version:

Implementation details

• include decimals mapping into _tokenContractAddressToTokenProperties to reduce storage allocation
• create custom ERC20PresetMinterPauser contract with customizable decimals for testing so we have a more real life example

Testing details

Unit and e2e tests

Acceptance Criteria

• Current unit and e2e passing with the logic change
• Add new tests that check ERC20 token decimals match decimal values saved on handlers

We want to make generic handler permissionless, where each developer can use Sygma infrastructure to execute cross-chain calls without needing to contact the Sygma team to register it beforehand.

For more details on implementation and more context check this notion page.

Implementation details

The generic handler should be refactored so it expects depositData to be formatted as defined below:

len(metaData):              uint256                         bytes 0   - 32
executeFuntionSignature:     bytes4    padded to 32 bytes   bytes 32  - 64
executeContractAddress       address   padded to 32 bytes   bytes 64  - 96
maxFee:                      uint256                        bytes 96  - 128
metaData:
    metadataDepositor:       address   padded to 32 bytes   bytes 128 - 156
    executionData:           bytes                          bytes 160 - END

On deposit function, the handler should validate metadataDepositor. Check if it is the same as the actual address that called deposit message on the Bridge contract.

On executeProposal, the handler should decode depositData and based on this data call function on receiving contract.

Note
For the first iteration of GenericHandler, we don't want to make changes to the Bridge contract, so we will still need an empty implementation of setResource so the new GenericHandler still fulfills IGenericHandler interface.

Testing details

• Test depositor validity is checked on deposit method
• Test execution is successfully decoded and executed
• Test migration scripts successfully deploy the new GenericHandler

Acceptance Criteria

• Passing unit tests
• Successful deployment of the new generic handler trough migration scripts

We decided to restructure handlers interfaces for maintainability reasons. The idea is to have a generic interface for all current and future handlers. Details of this refactor are described in generic handler documentation.

Implementation details

• Add a new IHandler interface
• Remove the old IGenericHandler and IDepositExecute interfaces.
• Refactor all existing handlers to use the new interface. This includes old (permissioned) generic handler.
• Refactor the Bridge contract, so it uses the generic adminSetResource function.
• Adjust unit tests accordingly.

Testing details

• Adjust current unit tests

Acceptance Criteria

• Implement described changes
• Passing unit tests

Set constant to prevent generic calls be larger than 1 000 000 gas.

Implementation details

Testing details

Acceptance Criteria

baseURI shadows an existing storage variable. The use of identical variable names for global and
internal variables can lead to confusion.

Implementation details

Use different names for global and internal variables.

Testing details

unit tests

Acceptance Criteria

• no compiler warning about shadowing
• passing unit and e2e tests

Acceptance criteria
[] Apply migrations to testnet and manually test
[] Write a checklist of actions and steps to apply changes on testnet
[] Apply these changes according to the checklist

When we deploy contracts to a new network, we register fee handlers on the fee router per domain, but the existing network doesn't know about the new network and doesn't have fee handlers set.

Expected Behavior

When deploying a new network, fee handlers for that domain on other networks should also be registered.

Current Behavior

Possible Solution

Steps to Reproduce (for bugs)

Versions

ChainBridge commit (or docker tag):
chainbridge-solidity version:
chainbridge-substrate version:
Go version:

Constructors have public visibility, but these are ignored and do not need to be defined since visibility for constructors has been deprecated, remove from following contracts:

• Bridge
• Forwarder
• Migrations
• FeeHandlerRouter
• PermissionedGenericHandler
• PermissionlessGenericHandler
• BasiFeeHandler
• AccessControSegregator

Implementation details

remove all public visibility from constructors

Testing details

n/a

Acceptance Criteria

no compiler warnings

Separate fee handler with oracle contracts for evm, substrate, and generic message passing as separate implementations.

Implementation details

Currently, we have this logic inside FeeHandlerWithOracle:

if(oracleMessage.msgGasLimit > 0) {
    // txCost = dstGasPrice * oracleMessage.msgGasLimit * Base Effective Rate (rate between base currencies of source and dest)
    txCost = oracleMessage.dstGasPrice * oracleMessage.msgGasLimit * oracleMessage.ber / 1e18;
} else {
    // txCost = dstGasPrice * _gasUsed * Token Effective Rate (rate of dest base currency to token)
    txCost = oracleMessage.dstGasPrice * _gasUsed * oracleMessage.ter / 1e18;
}

that determines if the processed message (deposit request) is just a basic token transfer or a generic message (msgGasLimit > 0).

This approach allows users to do funky things like set this msgGasLimit when requesting fee data from fee oracle, but then execute erc20 token bridging with this fee data. I don't know how exactly this can be used for malicious behavior, but we should split this into two different implementations of FeeHandlerWithOracle. With this approach, we can map exact implementation based on resourceID and domains inside fee router and not even worry about how this can be used for malicious activity.

Testing details

• Refactor and expand unit test

Acceptance Criteria

• Have three different implementations of fee handler with oracle
• Passing unit tests

Currently, IGenericHandler defines the function signature that should be used for registering resources.

function setResource(
        bytes32 resourceID,
        address contractAddress,
        bytes4 depositFunctionSig,
        uint depositFunctionDepositorOffset,
        bytes4 executeFunctionSig,
) external;

Considering our new generic handler, this interface doesn't make sense anymore.

Implementation details

TBD

Testing details

TBD

Acceptance Criteria

TBD

Expected Behavior

Expected behaviour should be to deploy one token handler per network and register all belonging tokens to that handler.

Current Behavior

Currently when redeploying and registering new handlers migration is executed it deploys and registers a new handler for each token.

Possible Solution

Move deploying new contract outside for loop.

Steps to Reproduce (for bugs)

Run migration script for redeploying token handlers.

setFeeOracle and setFeeProperties functions update important property variables but do not emit corresponding events.

Implementation details

Emit appropriate events on updating these properties.

Testing details

unit tests

Acceptance Criteria

events are emitted in tests

Implement fee whitelist on the fee handler router to exempt some protocols from paying fees.

Implementation details

Add whitelist to handler and enable setting with an admin function.

Testing details

Acceptance Criteria

setFeeOracle and setFeeProperties functions update important property variables but do not emit corresponding events.

Implementation details

Emit appropriate events on updating these properties.

Testing details

unit tests

Acceptance Criteria

events are emitted in tests