Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unidirectional ethereum-contracts #50

Closed
wants to merge 12 commits into from
Closed

Conversation

denalimarsh
Copy link

Changes to Peggy's ethereum contracts. Contracts only include functionality for unidirectional transfers of funds from Ethereum -> Cosmos and the branch is being actively used for testing transfer of assets from Ethereum to Cosmos. Existing work on secured unlocking in Valset.sol will be reincorporated once one-way transfers have been prototyped. If these changes are too breaking for master, wait and merge this PR once existing features have been reintegrated.

@zramsay zramsay requested review from sunnya97 and mossid May 16, 2019 21:58
@cwgoes
Copy link

cwgoes commented May 16, 2019

Thanks! Adding a written technical specification in Markdown would be helpful for review here.

@cwgoes cwgoes self-requested a review May 16, 2019 22:18
@denalimarsh
Copy link
Author

Thanks! Adding a written technical specification in Markdown would be helpful for review here.

Added a project specification in the README.md of the /ethereum-contracts directory.

@fedekunze
Copy link
Collaborator

cc: @okwme can you review as well ?


bytes32 hashData = keccak256(name, decimals);
require(Valset.verifyValidators(hashData, signers, v, r, s));
// Transfer item's funds and unlock it
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's an item in this context ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An item refers to a single incoming transaction containing funds to be locked. Originally named 'escrow', the terminology was updated to 'item' to prevent confusion regarding the fact that the funds are not technically held in escrow for the unidirectional implementation. Happy to update the naming conventions again, should there be consensus about a more suitable name for an instance of locked funds.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. We use the term Msg on the SDK (See sdk.Msg and auth.StdTx for reference). I think that's a more accurate term for this case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Items aren't 1:1 with Msgs, as Items serve as storage for the original sender's funds and include a unique id which isn't currently reflected in the Msg sent on Cosmos - but I agree that 'Msg' is a much more accurate name than 'Item'. @cwgoes what are your thoughts on the naming conventions?

@@ -1,5 +1,5 @@
{
"name": "ethereum-contracts",
"name": "ethreum-contracts",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo ?

Suggested change
"name": "ethreum-contracts",
"name": "ethereum-contracts",

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks for catching that.

Copy link

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick pass on the spec.

I'd like to see more detail on the individual steps involved in an Ethereum -> Cosmos transaction, and a list of the invariants which this bridge maintains (for example, that funds, once escrowed into the contract and minted on the Cosmos chain, cannot be un-escrowed - or, if that isn't the case, an explanation of what alternative property is being provided).


You can test the contracts using Truffle Testrpc in the console or Ganache UI.
### Goals of the Smart Contracts
1. Securely implement core functionality of the system such as asset locking and event emission without endangering any user funds. As such, this prototype does not permanently lock value and allows the original sender full access to their funds at any time.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "permanently lock"? Can the original sender unlock their original tokens even after they've sent tokens to Cosmos?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. The original sender can withdraw tokens using withdraw(bytes32 id) while the contract's relayer can return tokens to the item's original sender using unlock(id). This is intended behavior for this stage of the project to protect any user funds should someone attempt to deploy the contracts to the mainnet; once the bridge has undergone robust testing the withdraw() function will be removed entirely and the unlock() function will be contingent on approval from the validator set.

I've updated the project specification to state that this project specification focuses on the role of 'Peggy', the Smart Contract system deployed to the Ethereum network, and is meant to contextualize its role within the bridge. Specifications detailing the structure and step=-by-step process of the non-Ethereum components (Relayer service, EthOracle module, Oracle module) will soon be available in the cosmos-ethereum-bridge repository.

ethereum-contracts/README.md Outdated Show resolved Hide resolved

![alt text](./img/ganache_setup.png "Ganache Setup")
### System Process:
1. Users lock Ethereum or ERC20 tokens on the Peggy contract, resulting in the emission of an event containing the created item's original sender's Ethereum address, the intended recipient's Cosmos address, the type of token, the amount locked, and the item's unique nonce.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"created item" is not clear - what "item" is being referred to? What is the nonce? Is this a sort of packet?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, an Item is a struct containing information about a unique incoming transaction which executes the lock() function. Due to confusion about the name, we should rename the struct to something more descriptive. How about 'LockedTokens' or 'Deposit'?

The nonce is a global variable which protects Peggy against possible replay attacks so that an item's funds can only be unlocked once. Consider this attack scenario:

  • An adversary locks X ETH into the contract (move from Ethereum to Cosmos Zone).
  • He then requests the reverse operation (move from Cosmos Zone to Ethereum).
  • After collecting the validator signatures, the relayer creates a transaction calling the unlock function for the amount X to sent to the user.
  • The adversary observes the transaction on the blockchain, and copies the signatures
  • The adversary, having already received X ETH, creates another transaction calling unlock directly himself for the amount X and replaying the captured signatures.
  • The amount X is transferred to the adversary again
  • The attacker can repeat this attack until the contract has been drained of ETH.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deposit sounds great - and a nonce makes sense for replay prevention, agreed.

ethereum-contracts/README.md Outdated Show resolved Hide resolved
ethereum-contracts/README.md Show resolved Hide resolved

Copy it and on in a separate tab in the console:
## Other Considerations
We decided to temporarily remove the validator set from this version of Peggy, our reasoning being that system transparency and clarity should take precedence over the inclusion of future project features.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? That there still is a validator set - just not the same as the Tendermint validator set?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. This refers to the validator set from Valset.sol, which was removed from the Smart Contracts for the unidirectional implementation. I've updated the section to be more explicit.

ethereum-contracts/README.md Show resolved Hide resolved

## Installing dependencies
## Project Summary
Unidirectional Peggy is the starting point for cross chain value transfers from Ethereum to Cosmos as part of the Ethereum Cosmos Bridge project. The smart contract system accepts incoming transfers of Ethereum and ERC20 tokens, locking them while the transaction is validated and equitable funds issued to the intended recipient on Cosmos.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this part clarify that "Cosmos" is a specific bridge chain running Tendermint consensus and custom modules?

By itself, "Cosmos" refers to the network as a whole, not a particular blockchain.

Or is this intended to become an upgrade for the Cosmos Hub (which is a particular blockchain)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the custom modules will eventually be submitted to the Cosmos sdk as an upgrade for the Cosmos Hub and other blockchains Cosmos network. @jazzy and @jackzampolin do you have any insight on this?

@denalimarsh
Copy link
Author

Future requests for revisions should be directed to #51 and any outstanding comments from this PR will be addressed in #51 . This PR will be removed once the relevant revisions have been integrated in the completed unidirectional branch.

@denalimarsh
Copy link
Author

Closing this as #51 contains the up-to-date version of the smart contracts which include any requested changes from this PR.

jkilpatr pushed a commit that referenced this pull request Oct 16, 2020
@tac0turtle tac0turtle deleted the unidirectional branch March 11, 2021 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants