feat(trading-proto-upgrade): PoC #6

artemii235 · 2024-02-12T07:02:40Z

Implemented EtomicSwapV2 contract containing functions needed to support Trading Protocol Upgrade V2.

shamardy

Only one comment at this stage to understand the proposed workflow better :)

contracts/EtomicSwap.sol

laruh · 2024-02-18T05:49:36Z

I suppose we can remove merge script from package.json, as we dont use sol-merge lib bcz its unnecessary.
And change "clean": "rm -rf merge" to "clean": "npx hardhat clean".

https://github.com/KomodoPlatform/etomic-swap/blob/trading-proto-upgrade-v2-poc/package.json#L8-L9

laruh · 2024-02-18T05:56:35Z

@shamardy in dm with @artemii235 we discussed that I will add nft swap V2 methods in trading proto upgrade V2 branch.
But I see that PR is already open, will it be easier for you to review nft V2 methods in separate PR or its ok if I add nft swaps V2 implementation in trading proto V2 branch?

laruh

could you please change your tests ordering according current order in dev?

test/EtomicSwap.js

laruh

same for contract methods

contracts/EtomicSwap.sol

artemii235 · 2024-02-21T08:11:30Z

@shamardy @laruh
I had to do quite big refactoring to move new functionality to a separate smart contract. It's needed because we have hit contract size limit introduced in EIP-170. Could you review the PR again, please?

laruh

Thanks a lot for moving V2 ops to the new smart contract! Its better.

I have the first brief notes

test/EtomicSwap.js

laruh

more notes

contracts/EtomicSwapV2.sol

artemii235 · 2024-02-22T05:45:19Z

@laruh All notes are fixed, please review one more time 🙂

laruh · 2024-02-22T08:16:33Z

Thanks for the fixes!
@artemii235 I suggest to add additional layers of security.

First is to use SafeERC20 wrapper for ERC20
Reference: https://forum.openzeppelin.com/t/making-sure-i-understand-how-safeerc20-works/2940

SafeERC20 addresses the inconsistencies and safety concerns when interacting with ERC20 tokens that do not strictly adhere to the standard's expected return values. It wraps ERC20 operations to ensure that failures are handled correctly, preventing tokens from being permanently locked in contracts due to non-standard compliant return values. This utility is particularly valuable when dealing with a wide range of ERC20 tokens, including those that may not return a boolean value on success or failure, as expected per the ERC20 standard.
The forum discussions (link above) around SafeERC20 clarify its role in safely interacting with ERC20 tokens, ensuring that operations like transfers and approvals are executed as intended without risking the loss of tokens.

Second is to add ReentrancyGuard impl to EtomicSwapV2 and apply nonReentrant to erc20MakerPayment and erc20TakerPayment functions.
Ref:
https://docs.openzeppelin.com/contracts/5.x/api/utils#ReentrancyGuard
https://spin.atomicobject.com/reentrancy-guard-smart-contracts/
https://ethereum.stackexchange.com/questions/149283/when-should-i-use-reentrancyguard-in-a-smart-contract

The use of ReentrancyGuard is advocated to prevent reentrancy attacks, which are a critical vulnerability in smart contracts, especially those handling external calls that could be exploited to drain contract funds. ReentrancyGuard provides a straightforward and effective mechanism to prevent such attacks by using a simple state variable to block reentrant calls.
To sum up, ReentrancyGuard is an additional layer for CEI Pattern to prevent reentrancy attack.

So you will have this:

import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

contract EtomicSwapV2 is ReentrancyGuard {
    using SafeERC20 for IERC20;

    function erc20MakerPayment(
        bytes32 id,
        uint256 amount,
        address tokenAddress,
        address taker,
        bytes20 takerSecretHash,
        bytes20 makerSecretHash,
        uint32 paymentLockTime
    ) external nonReentrant {
    // code
    }
    
    function erc20TakerPayment(
        bytes32 id,
        uint256 amount,
        uint256 dexFee,
        address tokenAddress,
        address receiver,
        bytes20 takerSecretHash,
        bytes20 makerSecretHash,
        uint32 immediateRefundLockTime,
        uint32 paymentLockTime
    ) external nonReentrant {
    // code
    }

laruh

non blocking notes

laruh · 2024-02-22T10:08:49Z

test/EtomicSwapV2.js

+        await etomicSwapRunner0.erc20MakerPayment(...payment_params).should.be.rejectedWith("Maker payment is already initialized");
+    });
+
+    it('should allow taker to spend ETH maker payment', async function() {


Could you create etomicSwapRunners in this test please

laruh · 2024-02-22T10:09:56Z

test/EtomicSwapV2.js

+        await etomicSwapV2.connect(accounts[1]).spendMakerPayment(...spendParams).should.be.rejectedWith(INVALID_PAYMENT_STATE_SENT);
+    });
+
+    it('should allow taker to spend ERC20 maker payment', async function() {


Here too, use etomicSwap runners for 0 and 1 account, please

laruh

I have questions about Taker / Maker methods and their taker / maker secret params.

laruh · 2024-02-22T10:26:19Z

contracts/EtomicSwapV2.sol

+    function refundMakerPaymentTimelock(
+        bytes32 id,
+        uint256 amount,
+        address taker,
+        bytes20 takerSecretHash,
+        bytes20 makerSecretHash,
+        address tokenAddress
+    ) external {


Q1: Why in refundMakerPaymentTimelock function we use bytes20 takerSecretHash parameter, but in refundMakerPaymentSecret we provide bytes32 takerSecret (not hash)?

Q2: and actually both refundMakerPaymentSecret and refundTakerPaymentSecret utilize bytes32 takerSecret. why we dont use makerSecret in refund methods?

takerSecret and makerSecret are generated by Taker and Maker, respectively. takerSecret is used for immediate refund path, while makerSecret is used for spending path.

If timelock refund path is used, it typically means that taker didn't reveal his secret. So, takerSecretHash is used just to calculate paymentHash in the function.

contracts/EtomicSwapV2.sol

artemii235 · 2024-02-22T13:43:15Z

@laruh Could you please leave your notes in a single review? I don't really see a reason to create 3 review iterations in a day when it could be easily one.

artemii235 · 2024-02-22T13:52:56Z

@laruh
I will read more about SafeERC20 and likely add it to the contract.

As per ReentrancyGuard, our implementation already follows CEI pattern: it changes state and emits events before transferring ETH/calling other contracts. Making function nonReentrant also increases gas used by ~2500, so I don't think it is really needed.

shamardy

Thank you for the changes! Next review iteration where I have some notes/questions that might optimize the gas used by the contract a bit, I guess there can be other places for optimizations that I missed.

shamardy · 2024-02-22T17:52:38Z

contracts/EtomicSwapV2.sol

+                msg.sender,
+                maker,
+                takerSecretHash,
+                ripemd160(abi.encodePacked(sha256(abi.encodePacked(makerSecret)))),


Should we start using sha256 only to reduce gas consumption. We already support sha256 secret hash in the defi framework.
P.S. we should start taking gas reduction into consideration before deploying the new contract ref. KomodoPlatform/komodo-defi-framework#1857 (comment) . Also, if this issue KomodoPlatform/komodo-defi-framework#1857 can be solved without additional gas cost it would be good to do it in v2 contract since we couldn't do it for backward compatibility issues before.

Good note, I will test a case without additional ripemd160 call and post results here later.

So, with ripemd160, spendMakerPayment gas usage is 50697 (ERC-20), with 32 bytes secret hash (sha256-only): 49271. It comes with ~200 gas increase of erc20MakerPayment, but overall it should be beneficial. I pushed the changes.

contracts/EtomicSwapV2.sol

shamardy · 2024-02-22T20:14:29Z

contracts/EtomicSwapV2.sol

+                msg.sender,
+                takerSecretHash,
+                makerSecretHash,
+                address(0)


do we really need to make zero address part of eth payment hash? I guess if removed we would need to duplicate a lot of functions, e.g. 2 spendMakerPayment functions, one for erc20 and one for eth.

Yeah, having this address(0) allows using a single spend/refund function. So, if we remove it, we will have more boilerplate code with questionable benefit, IMHO 🙂

contracts/EtomicSwapV2.sol

laruh · 2024-02-23T09:44:17Z

@shamardy @laruh I had to do quite big refactoring to move new functionality to a separate smart contract. It's needed because we have hit contract size limit introduced in EIP-170. Could you review the PR again, please?

Hmmm I realized that adding nft swap V2 methods in EtomicSwapV2 can lead to the same issue.

artemii235 · 2024-02-23T12:18:35Z

@laruh Yeah, I propose to create a separate EtomicSwapNFT contract as workaround 🙂

laruh

Next review iteration

contracts/EtomicSwapV2.sol

artemii235 · 2024-02-26T11:27:40Z

@laruh @shamardy I've fixed all review notes 🙂

Please note that SafeERC20 usage increases gas consumption:

erc20MakerPayment uses 92233 gas (~1200 increase).
spendMakerPayment uses 50418 gas (Also ~1200 increase).

laruh · 2024-02-26T13:40:10Z

contracts/EtomicSwapV2.sol

+    event TakerPaymentSent(bytes32 id);
+    event TakerPaymentApproved(bytes32 id);
+    event TakerPaymentSpent(bytes32 id, bytes32 secret);
+    event TakerPaymentRefundedSecret(bytes32 id, bytes32 secret);


Q: you removed takerSecret from event MakerPaymentRefundedSecret, but left it in event TakerPaymentRefundedSecret. Is it in purpose? or you just missed it?

Related #6 (comment), we only need it for taker refund so that the maker can use it to refund their payment.

laruh

LGTM!

shamardy

Last review from my side! Just some nits and one question/suggestion!

contracts/EtomicSwapV2.sol

artemii235 · 2024-02-27T13:13:47Z

@shamardy Review notes are fixed 🙂

shamardy

🔥

shamardy · 2024-02-27T20:25:47Z

contracts/EtomicSwapV2.sol

+        if (takerPayments[id].state == TakerPaymentState.TakerApproved) {
+            require(
+                block.timestamp >= takerPayments[id].paymentLockTime,
+                "Current timestamp didn't exceed payment refund lock time"
+            );
+        }
+
+        if (takerPayments[id].state == TakerPaymentState.PaymentSent) {
+            require(
+                block.timestamp >= takerPayments[id].preApproveLockTime,
+                "Current timestamp didn't exceed payment pre-approve lock time"
+            );
+        }


It's ironic that after adding this change I can see that there is no actual need for timelock refund if TakerPaymentState is PaymentSent since refund using taker secret can work anytime when TakerPaymentState is PaymentSent, this at least adds a mitigation in case the taker loses their secret due to any problem which was one of the reasons we added timelocked refund for the funding transaction when designing the protocol IIRC. Just a note and not a change request :)

Artem Vitae added 2 commits February 7, 2024 12:05

Copy code from old branch.

b679c76

Add test spending ERC20 v2 payment and fix bug.

9ec27e2

artemii235 requested review from shamardy and laruh February 12, 2024 07:02

shamardy reviewed Feb 16, 2024

View reviewed changes

contracts/EtomicSwap.sol Outdated Show resolved Hide resolved

laruh force-pushed the trading-proto-upgrade-v2-poc branch from 64b99db to 9ec27e2 Compare February 18, 2024 05:44

laruh requested changes Feb 18, 2024

View reviewed changes

test/EtomicSwap.js Show resolved Hide resolved

laruh requested changes Feb 18, 2024

View reviewed changes

contracts/EtomicSwap.sol Outdated Show resolved Hide resolved

Artem Vitae added 2 commits February 20, 2024 18:17

WIP. Adding a new EtomicSwapV2 contract.

888d11c

Finished EtomicSwapV2 with tests.

c1bbd40

laruh requested changes Feb 21, 2024

View reviewed changes

test/EtomicSwap.js Outdated Show resolved Hide resolved

test/EtomicSwap.js Outdated Show resolved Hide resolved

laruh requested changes Feb 21, 2024

View reviewed changes

contracts/EtomicSwapV2.sol Outdated Show resolved Hide resolved

contracts/EtomicSwapV2.sol Outdated Show resolved Hide resolved

contracts/EtomicSwapV2.sol Outdated Show resolved Hide resolved

contracts/EtomicSwapV2.sol Outdated Show resolved Hide resolved

Artem Vitae added 2 commits February 22, 2024 12:41

Review fixes.

d4fa440

Review fixes.

a53e50b

laruh reviewed Feb 22, 2024

View reviewed changes

shamardy reviewed Feb 22, 2024

View reviewed changes

laruh requested changes Feb 26, 2024

View reviewed changes

contracts/EtomicSwapV2.sol Outdated Show resolved Hide resolved

contracts/EtomicSwapV2.sol Outdated Show resolved Hide resolved

contracts/EtomicSwapV2.sol Show resolved Hide resolved

Artem Vitae added 3 commits February 26, 2024 17:54

Review fixes.

687456e

Use sha256 only for secret hashing.

1a07b69

Use SafeERC20.

4b03c4e

laruh reviewed Feb 26, 2024

View reviewed changes

laruh approved these changes Feb 26, 2024

View reviewed changes

shamardy reviewed Feb 26, 2024

View reviewed changes

contracts/EtomicSwapV2.sol Outdated Show resolved Hide resolved

contracts/EtomicSwapV2.sol Show resolved Hide resolved

contracts/EtomicSwapV2.sol Outdated Show resolved Hide resolved

Review fixes.

a7c98d3

shamardy approved these changes Feb 27, 2024

View reviewed changes

shamardy merged commit 124445a into dev Feb 27, 2024

shamardy deleted the trading-proto-upgrade-v2-poc branch February 27, 2024 20:30

shamardy mentioned this pull request Jul 20, 2024

feat(nft-swap): complete refund methods KomodoPlatform/komodo-defi-framework#2129

Merged

feat(trading-proto-upgrade): PoC #6

feat(trading-proto-upgrade): PoC #6

Conversation

artemii235 commented Feb 12, 2024 • edited Loading

shamardy left a comment

Choose a reason for hiding this comment

laruh commented Feb 18, 2024 • edited Loading

laruh commented Feb 18, 2024 • edited Loading

laruh left a comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

artemii235 commented Feb 21, 2024

laruh left a comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

artemii235 commented Feb 22, 2024

laruh commented Feb 22, 2024 • edited Loading

laruh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemii235 commented Feb 22, 2024

artemii235 commented Feb 22, 2024

shamardy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh commented Feb 23, 2024

artemii235 commented Feb 23, 2024

laruh left a comment

Choose a reason for hiding this comment

artemii235 commented Feb 26, 2024

laruh Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

shamardy Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

artemii235 commented Feb 27, 2024

shamardy left a comment

Choose a reason for hiding this comment

shamardy Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

artemii235 commented Feb 12, 2024 •

edited

Loading

laruh commented Feb 18, 2024 •

edited

Loading

laruh commented Feb 18, 2024 •

edited

Loading

laruh commented Feb 22, 2024 •

edited

Loading

laruh Feb 26, 2024 •

edited

Loading

shamardy Feb 26, 2024 •

edited

Loading

shamardy Feb 27, 2024 •

edited

Loading