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

ERC20/ETH Htlc log needs to emit secret #638

Closed
bonomat opened this Issue Jan 11, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@bonomat
Copy link
Member

bonomat commented Jan 11, 2019

While doing #412 a security issue was found:

We cannot rely on the fact that the transaction redeeming an HTLC:
a) was sent to our HTLC
b) nor contains the secret in the data field

For example, one could create another contract (contract A), which calls the HTLC contract (contract B) in order to redeem it. The transaction invoking contract A might not have the secret as data, and the call() to contract B is not tracked in the transaction receipt.

Hence, our HTLCs need to log the secret when it was successfully redeemed.

Blocked by #756

@bonomat

This comment has been minimized.

Copy link
Member Author

bonomat commented Feb 1, 2019

Please note the scope of this ticket has changed:

  • ERC20/ETH contracts are emitting the secret when redeemed
  • LQS needs to return TransactionReceipt instead of Transaction when ?expand_results=true is requested
  • LQS client needs to handle the TransactionReceipt and extract the secret from its logs

Doing this ticket without #639 probably does not make much sense.

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Feb 15, 2019

With #756 this is how you should go about doing this ticket:
Make LQS return BOTH the TransactionReceipt AND the Transaction when ?expand_results=true (note preferably just make two show_transaction=true&show_receipt=true)

You should start here: https://github.com/comit-network/comit-rs/tree/638-emit-secret-in-htlc (and rebase on to master).

Also please get the amount of tokens actually transferred from the receipt and return it as part of the htlc_funded future.

@LLFourn LLFourn assigned thomaseizinger and unassigned LLFourn Feb 15, 2019

@D4nte

This comment has been minimized.

Copy link
Member

D4nte commented Feb 17, 2019

@LLFourn I don't agree re token amount. The smart contract emitting "redeemed" should be enough to know that all token were transferred as we know that the smart contract code is correct.
We trust the smart contract code and we trust LQS to find the correct one.
We should not need to double check the token amount on comit_node side.

@bonomat

This comment has been minimized.

Copy link
Member Author

bonomat commented Feb 17, 2019

If we are fine with having this trust in our system then we don't need it.
I would opt for a verify don't trust approach though.
It's not so complicated to do verify the amount, the data is in the receipt and it does not cost us any performance as it's a simple equal check.

@D4nte

This comment has been minimized.

Copy link
Member

D4nte commented Feb 18, 2019

Ok, I did not read properly. Yes, agreed to have the amount as part of the funding future. I thought we wanted it as part of the redeem event as this issue is about redeeming...

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Feb 26, 2019

Fixed in #784.

@wafflebot wafflebot bot removed the review label Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.