multi: Add ETHSwapV0. #1019

JoeGruffins · 2021-03-19T09:56:24Z

Have the eth harness start with a contract. Add contract bindings and
basic calls to clients.

JoeGruffins · 2021-05-05T08:41:14Z

Is this test failure #1018 ?

server/asset/eth/contracts/ETHSwapV0.sol

chappjc · 2021-07-20T03:13:27Z

You may rebase with PR #1078 in so we can give it a review.

JoeGruffins · 2021-07-20T07:15:27Z

client/asset/eth/rpcclient_harness_test.go

+// TODO: Occasionally tests will fail with "timed out" because transactions are
+// not being mined. If one tx is not mined, following txs from the same account
+// with a higher nonce also cannot be mined, so it causes all tests after to
+// fail as well. Find out why the first tx fails to be broadcast.


This seems to be unrelated to #1130

Possibly realated...

ethereum/go-ethereum#21385

I've spent a bit of time on this, but unable to stop this from happening so far.

Headway on this. After adding better logging, the server was telling us:
TRACE[07-28|11:37:15.770] Discarding invalid transaction hash=5a1f01..40f83e err="nonce too low"

Which suggests that the light client sometimes has trouble discerning the correct nonce. Probably because we destroy its persistent data every test suite.

JoeGruffins · 2021-07-20T07:21:30Z

You may rebase with PR #1078 in so we can give it a review.

Srry, stuck another one in front.

chappjc · 2021-07-23T21:06:20Z

dex/networks/eth/contracts/ETHSwapV0.sol

+        payable(msg.sender).transfer(swaps[secretHash].value);
+        swaps[secretHash].state = State.Redeemed;
+        swaps[secretHash].secret = secret;
+    }
+
+    function refund(bytes32 secretHash)
+        public
+        isRefundable(secretHash, msg.sender)
+    {
+        payable(msg.sender).transfer(swaps[secretHash].value);


Big 👍 for msg.sender and not tx.origin. But I wonder if we should outright prevent indirect calls by a proxy contract, say with something like require(tx.origin == msg.sender) to restrict the caller to being a regular "codeless" address.

Ref: https://github.com/ethereumbook/ethereumbook/blob/develop/09smart-contracts-security.asciidoc#txorigin-authentication

Lets do that.

The purpose of this pr is mostly to get more stuff hooked up. I would not be suprised at all if there are vulns in the contract. It is mostly a stripped down version of the original at https://github.com/decred/atomicswap/blob/2f6679b9150b2ef364cd7bc75a49c21a7d124a91/cmd/ethatomicswap/contract/src/contracts/AtomicSwap.sol

I make no claim that is safe at this point.

Indeed, we're working toward PoC. The work proposed to the atomicswap repo is absolutely the right starting point, and done by ETH oriented developers. I think we should all agree that none of this will be default-enabled on mainnet until much farther along in the process, and maybe not at all without outside consultation and extended testing... such is the nature of ETH. We definitely need to be clear about that.

But I have been meaning to discuss other contract related concerns I've been pondering, ranging from specific lessons learned in recent weeks, to higher level decisions like using a single contract for EVERYONE. I personally think this is one of the mistakes made by not just thor but eth and defi in general -- putting all the eggs in one basket with a single fallible contract. It was a hard lesson for The DAO years ago, and again with the Thor Router. (don't just hack one person, hack all involved)

In UTXO-land we make a unique contract for each swap, and while I get that a single reusalble contract published by a third party can have utility in many ways including saving on tx fees, it does make me uneasy... from a security point of view as well as bringing a third party (devs / contract publishers) into the mix.

Just something I'd like to consider in the future. Nothing to hold up this work though since as you've said we're building the ETH framework for DCRDEX. The contract we make now is not meant to be the perfect solution for product. (EDIT: not meant to be)

Interesting.

I hope you don't mine me quoting this for the contract issue.

No, not at all. I meant to be explicit about this. This is a process and PR 1019 isn't the end-all.

JoeGruffins · 2021-07-28T05:16:09Z

So far unable to hit several errors that I was seeing in tests when using the same internal node directory. Please let me know of any random failures.

chappjc · 2021-07-28T14:18:25Z

dex/networks/eth/contracts/ETHSwapV0.sol

+        senderIsOrigin()
+        isRedeemable(secretHash, secret, msg.sender)
+    {
+        payable(msg.sender).transfer(swaps[secretHash].value);


Maybe it doesn't apply to us, but I was looking into send/transfer/call, and there is a recommendation to stop using call or transfer, and instead use call. https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now, https://consensys.github.io/smart-contract-best-practices/recommendations/#dont-use-transfer-or-send, and https://ethereum.stackexchange.com/a/78136

send/transfer being introduced after The DAO hack to mitigate reentrancy exploits seems to be seen as a half-measure at guarding reentrancy (by starving external calls of gas), plus the possibility of operations changing gas costs (see istanbul/SLOAD) makes transfer not recommended.

Although we're not making state updates to a "user balance" map, I'd be concerned that ~~either (a) the transfer could use up all the gas before storing the secret, thus screwing over the counterparty, or (b)~~ the receiving account payable function would call back to redeem and get paid again. I think prevailing advice for mitigating reentrancy is the checks-effects-interactions pattern, whereby you put external calls after all state changes (i.e. so that isRedeemable will prevent another send if state has already been updated before the external call that could call back to this function). This pattern seems to be preferred to a mutex these days (https://consensys.github.io/smart-contract-best-practices/known_attacks/#reentrancy).

It looks like we need to update the state, i.e. swaps[secretHash].state = State.Redeemed; before sending funds and that should prevent any replay attacks here, so any amount of gas would be fine. ofc no expert tho.

Ah but wait, if sending then failed, the funds would be locked forever... Or would failure sending revert the state?

Although we're not making state updates to a "user balance" map

I think we pretty much are, and in it's current state it looks like a redeemer or even refunder could indeed drain the contract using a replay attack with enough gas, which transfer, and it looks like send too, limits. The difference between the two being that transfer throws an exception on error while send just return false.

Ah but wait, if sending then failed, the funds would be locked forever... Or would failure sending revert the state?

That's what I thought. The txn would fail, and the external call be reverted. Guess it needs a test.
Hmm, I suppose issue (a) in my original comment isn't so much the concern as (b) draining the contract with a reentrant call.

I'm on it with a solution and a test.

Nice. But I'm far less concerned about transfer->call advice than about preventing reentrancy attacks, which I think we'd accomplish with updating state before external calls. In it's current form an attacker could indeed drain the entire contract given enough gas and their own contract with a fallback function that called back to our contract's redeem. This is just the kind of thing I meant about defi liking to put all the eggs in one basket, but I don't have a better suggestion at the moment (aside from each trade publishing a new contract like we do with utxos).

I took a deep delve into the contract and am feeling pretty confident now.

chappjc · 2021-07-28T14:23:30Z

dex/networks/eth/contracts/ETHSwapV0.sol

+    }
+
+    modifier isNotInitiated(bytes32 secretHash) {
+        require(swaps[secretHash].state == State.Empty);


~~curious why checking like this when I feel swaps[secretHash] should just be verified to be null~~ NVM, I see that these are like virtual entries when they don't exists, with the zero value of the type returned.

vctt94

Hey @JoeGruffins, So when I try to add the alpha ethereum wallet from harness, I got the following:

Not sure if I am doing something wrong here, have you seen this?

Have the eth harness start with a contract. Add contract bindings and basic calls to clients.

JoeGruffins · 2021-08-02T09:28:02Z

Have not looked into vctt's issue yet but will soon. It should not be pointed to any active nodes though, it should create a node at an empty location and reuse that.

chappjc

Awesome work doing a PoC for the reentrant contract vulnerability. Just some conceptual questions about testing the vuln contract, and the possibility for public read-only debugging methods.

chappjc · 2021-08-02T15:19:37Z

dex/networks/eth/contracts/ETHSwapV0.sol

    modifier hasNoNilValues(uint refundTime) {
        require(msg.value > 0);
        require(refundTime > 0);
        _;
    }

+    // senderIsOrigin ensures that this contract cannot be used by other


Is "other contracts" is really "proxy contracts" since there still seems to be no prohibition on the sender's account type (contract vs. codeless acct). Or... would a contract calling the swapper contract imply there was a regular codeless account as the origin this making any contract caller a proxy contract?

At present, I don't think contracts can actively do anything. They need a normal account to perform methods. The original account is always a human/normal account afaict. I really tried to find a way to spoof origin in the reentry attack but could not. If it is possible it is not common knowledge.

chappjc · 2021-08-02T15:22:04Z

dex/networks/eth/contracts/ETHSwapV0.sol

+    // isRefundable checks that a swap can be refunded. The requirements are
+    // the initiator is msg.sender, the state is Filled, and the block
+    // timestamp be after the swap's stored refundBlockTimestamp.
+    modifier isRefundable(bytes32 secretHash) {


Was removing the address arg from isRefundable and isRedeemable strictly necessary or did it have significant security improvements or did it become redundant with the origin/sender checks? Seems allowing a third party or even a block explorer to debug would be nice. Actually, with modifier, are methods not going to be read only interactions that you can do without txn? Sorry for the naive questions here, but I'm just thinking about the possibility for methods that you can just hit with say etherescan.io's Read Contract feature for debugging.

The reasoning for removing was simply because it didn't seem necessary as we are just sending msg.sender anyway. So, for simplicity.

I have not used etherscan.io's read contract feature... I will go check it out. However, msg.sender should be public as part of the initial transaction?

e.g. some random contract https://etherscan.io/address/0xb1e9157c2fdcc5a856c8da8b2d89b6c32b3c1229#readContract

You can do stuff like balanceOf(account) in the explorer without messing with transactions

It looks like you can submit code to get some of that debugging. You can see that this contract is not set up to do that: https://etherscan.io/address/0x7f3a4085dcdccd5d52249dbf604ba9236b433040#code

I think we could make some separate public view functions that inform about states, and so would be visible and useable by an explorer. I read somewhere that public functions shouldn't call other public functions. Probably because of vulnerabilities like reentry attack creeping in undetected. Will look into it some more, but separate public functions for debugging like you say should be doable and fine, if you think it would be good.

I'm sure we'll want such methods, and the contract source will be published and probably submitted too, but it's not something to hold up this PR anyway.
I expect these separate public functions won't add to gas cost for regular swaps, just the tx that publishes the actual contract, so I figure it's alright to add any such functions to the contract interface, right?

Yes, I think that the gas cost for the initial contract would be higher, probably not so much higher, but it shouldn't affect other methods at all.

chappjc · 2021-08-02T15:55:37Z

client/asset/eth/rpcclient_harness_test.go

+	// If the exploit worked, the test will fail here, with all funds
+	// drained from the contract.
+	if bal.Cmp(wantBal) != 0 {


So to see the exploit actually hit these paths, you'd make bindings for the vulnerable contract like

abigen --sol ./reentryattack/VulnerableToReentryAttack.sol --pkg eth --out contract.go

and then run the test, which would be deploying the vulnerable contract?

You would then need to replace the contract bin hex in the test harness with the created contract.go, minus the leading 0x, and restart the harness.

Currently we use the contract deployed in the harness to test. We could also deploy a new contract per test.

I could add a test that also deploys this contract separately to test that the vulnerability contract does work on a vulnerable contract. I didn't know if that was going too far though, as a use of time I mean.

Also, --out should point to the contract in dex/networks/eth. I actually meant to write a small Readme, so I think I'll do that.

No need to test the vuln contact forever. Just wondering

JoeGruffins · 2021-08-03T06:17:56Z

@vctt94 This should work now. I wasn't able to observe your issue specifically, but there was some missing logic for creating an account. Added a very temporary function to add a testing account that has funds. I have not tried on testnet, but anyway you should point the node to an empty directory, default should be fine, which is $HOME/.dexethclient

I notice a display issue that I won't try to correct here, but Balance is in gwei which is 1e9th of an eth. This should say 5,000 eth.

It looks like the UI assumes atoms in a few places.

Also, nothing else should work as of this pr, just initial set-up.

vctt94

Hey joe, yeah, it does create a new account now, on an empty dir, but yeah not many UI things working right now.

Will give it another round of tests later. Good job on this PR so far!

chappjc

LGTM. This PR was super productive IMO. I think we've learned a ton about ETH in general from you @JoeGruffins and about contract security. Pretty cool we were actually able to drain the proposed (but modified) contract from decred/atomicswap#76 because it also did not set state prior to making external calls (https://github.com/decred/atomicswap/pull/76/files#diff-bbbe323063803d4430a2f007b2e39e9cdefae18521d8d28664686728278db38aR158-R161)

chappjc · 2021-08-03T16:33:16Z

Also thanks for your testing @vctt94. Very helpful, please keep it up.

JoeGruffins · 2021-08-04T01:47:08Z

OK, but out of curiosity, is there no rpc to create a new account and return the address and private key?

There is! But for testing it's convenient to have funds, and for mainnet I think we will use an account based on the app seed so it can be recreated.

xaur · 2021-09-05T22:03:38Z

we were actually able to drain the proposed (but modified) contract from decred/atomicswap#76 because it also did not set state prior to making external calls

If there is a vuln in that PR it might be worth posting a summary in it. The PR looks dead but just in case.

chappjc · 2021-09-05T22:14:26Z

Even though it didn't follow best practice, theoretically permitting reentrant calls, the send instruction should limit gas in most cases.
Not as bad as it could have been.

JoeGruffins force-pushed the deploycontract branch from dde1eaa to 0a45cb0 Compare March 22, 2021 05:19

JoeGruffins mentioned this pull request Mar 23, 2021

eth: Solidity atomic swap contract discussion. #1001

Closed

JoeGruffins force-pushed the deploycontract branch from 0a45cb0 to bc9890b Compare March 24, 2021 07:22

JoeGruffins force-pushed the deploycontract branch 2 times, most recently from 805e2a7 to 1d030df Compare May 5, 2021 07:48

JoeGruffins force-pushed the deploycontract branch from 1d030df to e93c9c1 Compare May 12, 2021 01:13

martonp reviewed Jun 9, 2021

View reviewed changes

server/asset/eth/contracts/ETHSwapV0.sol Outdated Show resolved Hide resolved

This was referenced Jul 14, 2021

multi: Add internal ethereum clients. #1005

Merged

client/asset/eth: Use gwei in Balance. #1078

Merged

JoeGruffins force-pushed the deploycontract branch from e93c9c1 to 8e77378 Compare July 16, 2021 07:27

chappjc added the ETH label Jul 20, 2021

JoeGruffins force-pushed the deploycontract branch from 8e77378 to 223e952 Compare July 20, 2021 07:06

JoeGruffins commented Jul 20, 2021

View reviewed changes

JoeGruffins force-pushed the deploycontract branch from 223e952 to 3450a12 Compare July 23, 2021 07:02

chappjc reviewed Jul 23, 2021

View reviewed changes

JoeGruffins force-pushed the deploycontract branch 2 times, most recently from 075782e to 53d3cd1 Compare July 26, 2021 03:51

JoeGruffins marked this pull request as ready for review July 26, 2021 04:05

JoeGruffins force-pushed the deploycontract branch from 53d3cd1 to deebc82 Compare July 28, 2021 02:28

chappjc requested changes Jul 28, 2021

View reviewed changes

vctt94 reviewed Jul 29, 2021

View reviewed changes

JoeGruffins added 4 commits August 2, 2021 09:59

multi: Add ETHSwapV0.

a9afe71

Have the eth harness start with a contract. Add contract bindings and basic calls to clients.

Fix node grammar.

a4ab9d7

Persist test node.

23e6f1e

Add ReentryAttackTest.

775ae96

JoeGruffins force-pushed the deploycontract branch from 98adf63 to 775ae96 Compare August 2, 2021 09:15

chappjc reviewed Aug 2, 2021

View reviewed changes

JoeGruffins added 2 commits August 3, 2021 11:39

Add vulnerable contract usage.

9e74717

Add testing account.

8e78605

vctt94 reviewed Aug 3, 2021

View reviewed changes

chappjc added this to the 0.3 milestone Aug 3, 2021

chappjc approved these changes Aug 3, 2021

View reviewed changes

chappjc merged commit a7046c6 into decred:master Aug 3, 2021

multi: Add ETHSwapV0. #1019

multi: Add ETHSwapV0. #1019

Conversation

JoeGruffins commented Mar 19, 2021

JoeGruffins commented May 5, 2021

chappjc commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

JoeGruffins commented Jul 20, 2021

Choose a reason for hiding this comment

JoeGruffins Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

JoeGruffins commented Jul 28, 2021

chappjc Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

vctt94 left a comment

Choose a reason for hiding this comment

JoeGruffins commented Aug 2, 2021

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins commented Aug 3, 2021 • edited Loading

vctt94 left a comment

Choose a reason for hiding this comment

chappjc left a comment • edited Loading

Choose a reason for hiding this comment

chappjc commented Aug 3, 2021

JoeGruffins commented Aug 4, 2021

xaur commented Sep 5, 2021

chappjc commented Sep 5, 2021

JoeGruffins Jul 28, 2021 •

edited

Loading

JoeGruffins Jul 26, 2021 •

edited

Loading

chappjc Jul 26, 2021 •

edited

Loading

chappjc Jul 26, 2021 •

edited

Loading

chappjc Jul 28, 2021 •

edited

Loading

JoeGruffins Jul 29, 2021 •

edited

Loading

chappjc Jul 29, 2021 •

edited

Loading

chappjc Jul 29, 2021 •

edited

Loading

chappjc Jul 28, 2021 •

edited

Loading

JoeGruffins Aug 3, 2021 •

edited

Loading

JoeGruffins commented Aug 3, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading