From a6519fb280a9412f4e74cee19421ed15d895b9b2 Mon Sep 17 00:00:00 2001 From: Alexey Date: Thu, 14 Nov 2019 20:49:34 +0300 Subject: [PATCH 1/2] move nonReentrant modifier --- contracts/ERC20Mixer.sol | 4 ++-- contracts/ETHMixer.sol | 4 ++-- contracts/Mixer.sol | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/ERC20Mixer.sol b/contracts/ERC20Mixer.sol index 92677f1..144c786 100644 --- a/contracts/ERC20Mixer.sol +++ b/contracts/ERC20Mixer.sol @@ -26,12 +26,12 @@ contract ERC20Mixer is Mixer { token = _token; } - function _processDeposit() internal nonReentrant { + function _processDeposit() internal { require(msg.value == 0, "ETH value is supposed to be 0 for ERC20 mixer"); _safeErc20TransferFrom(msg.sender, address(this), denomination); } - function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) internal nonReentrant { + function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) internal { require(msg.value == _refund, "Incorrect refund amount received by the contract"); _safeErc20Transfer(_recipient, denomination - _fee); diff --git a/contracts/ETHMixer.sol b/contracts/ETHMixer.sol index bc2859b..2996b5e 100644 --- a/contracts/ETHMixer.sol +++ b/contracts/ETHMixer.sol @@ -22,11 +22,11 @@ contract ETHMixer is Mixer { ) Mixer(_verifier, _denomination, _merkleTreeHeight, _operator) public { } - function _processDeposit() internal nonReentrant { + function _processDeposit() internal { require(msg.value == denomination, "Please send `mixDenomination` ETH along with transaction"); } - function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) internal nonReentrant { + function _processWithdraw(address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) internal { // sanity checks require(msg.value == 0, "Message value is supposed to be zero for ETH mixer"); require(_refund == 0, "Refund value is supposed to be zero for ETH mixer"); diff --git a/contracts/Mixer.sol b/contracts/Mixer.sol index 039de94..a17f827 100644 --- a/contracts/Mixer.sol +++ b/contracts/Mixer.sol @@ -59,7 +59,7 @@ contract Mixer is MerkleTreeWithHistory, ReentrancyGuard { @dev Deposit funds into mixer. The caller must send (for ETH) or approve (for ERC20) value equal to or `denomination` of this mixer. @param _commitment the note commitment, which is PedersenHash(nullifier + secret) */ - function deposit(bytes32 _commitment) external payable { + function deposit(bytes32 _commitment) external payable nonReentrant { require(!commitments[_commitment], "The commitment has been submitted"); uint32 insertedIndex = _insert(_commitment); @@ -80,7 +80,7 @@ contract Mixer is MerkleTreeWithHistory, ReentrancyGuard { - the recipient of funds - optional fee that goes to the transaction sender (usually a relay) */ - function withdraw(bytes calldata _proof, bytes32 _root, bytes32 _nullifierHash, address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) external payable { + function withdraw(bytes calldata _proof, bytes32 _root, bytes32 _nullifierHash, address payable _recipient, address payable _relayer, uint256 _fee, uint256 _refund) external payable nonReentrant { require(_fee <= denomination, "Fee exceeds transfer value"); require(!nullifierHashes[_nullifierHash], "The note has been already spent"); require(isKnownRoot(_root), "Cannot find your merkle root"); // Make sure to use a recent one From ce550eea587e52b590c50591a58a8550edb8efdf Mon Sep 17 00:00:00 2001 From: Alexey Date: Fri, 15 Nov 2019 10:59:06 +0300 Subject: [PATCH 2/2] return refund to a relayer in case of failure --- contracts/ERC20Mixer.sol | 7 ++- contracts/Mocks/BadRecipient.sol | 7 +++ test/ERC20Mixer.test.js | 84 +++++++++++++++++++++++++++++++- 3 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 contracts/Mocks/BadRecipient.sol diff --git a/contracts/ERC20Mixer.sol b/contracts/ERC20Mixer.sol index 144c786..795d1c9 100644 --- a/contracts/ERC20Mixer.sol +++ b/contracts/ERC20Mixer.sol @@ -38,8 +38,13 @@ contract ERC20Mixer is Mixer { if (_fee > 0) { _safeErc20Transfer(_relayer, _fee); } + if (_refund > 0) { - _recipient.call.value(_refund)(""); + (bool success, ) = _recipient.call.value(_refund)(""); + if (!success) { + // let's return _refund back to the relayer + _relayer.transfer(_refund); + } } } diff --git a/contracts/Mocks/BadRecipient.sol b/contracts/Mocks/BadRecipient.sol new file mode 100644 index 0000000..a058b8b --- /dev/null +++ b/contracts/Mocks/BadRecipient.sol @@ -0,0 +1,7 @@ +pragma solidity ^0.5.0; + +contract BadRecipient { + function() external { + require(false, "this contract does not accept ETH"); + } +} diff --git a/test/ERC20Mixer.test.js b/test/ERC20Mixer.test.js index 6cbaf0b..30a178f 100644 --- a/test/ERC20Mixer.test.js +++ b/test/ERC20Mixer.test.js @@ -9,6 +9,7 @@ const { toBN, toHex } = require('web3-utils') const { takeSnapshot, revertSnapshot } = require('../lib/ganacheHelper') const Mixer = artifacts.require('./ERC20Mixer.sol') +const BadRecipient = artifacts.require('./BadRecipient.sol') const Token = artifacts.require('./ERC20Mock.sol') const USDTToken = artifacts.require('./IUSDT.sol') const { ETH_AMOUNT, TOKEN_AMOUNT, MERKLE_TREE_HEIGHT, ERC20_TOKEN } = process.env @@ -54,6 +55,7 @@ contract('ERC20Mixer', accounts => { let mixer let token let usdtToken + let badRecipient const sender = accounts[0] const operator = accounts[0] const levels = MERKLE_TREE_HEIGHT || 16 @@ -63,7 +65,7 @@ contract('ERC20Mixer', accounts => { let tree const fee = bigInt(ETH_AMOUNT).shr(1) || bigInt(1e17) const refund = ETH_AMOUNT || '1000000000000000000' // 1 ether - const recipient = getRandomRecipient() + let recipient = getRandomRecipient() const relayer = accounts[1] let groth16 let circuit @@ -83,6 +85,7 @@ contract('ERC20Mixer', accounts => { token = await Token.deployed() await token.mint(sender, tokenDenomination) } + badRecipient = await BadRecipient.new() snapshotId = await takeSnapshot() groth16 = await buildGroth16() circuit = require('../build/circuits/withdraw.json') @@ -201,6 +204,85 @@ contract('ERC20Mixer', accounts => { isSpent.should.be.equal(true) }) + it('should return refund to the relayer is case of fail', async () => { + const deposit = generateDeposit() + const user = accounts[4] + recipient = bigInt(badRecipient.address) + await tree.insert(deposit.commitment) + await token.mint(user, tokenDenomination) + + const balanceUserBefore = await token.balanceOf(user) + await token.approve(mixer.address, tokenDenomination, { from: user }) + await mixer.deposit(toFixedHex(deposit.commitment), { from: user, gasPrice: '0' }) + + const balanceUserAfter = await token.balanceOf(user) + balanceUserAfter.should.be.eq.BN(toBN(balanceUserBefore).sub(toBN(tokenDenomination))) + + const { root, path_elements, path_index } = await tree.path(0) + // Circuit input + const input = stringifyBigInts({ + // public + root, + nullifierHash: pedersenHash(deposit.nullifier.leInt2Buff(31)), + relayer, + recipient, + fee, + refund, + + // private + nullifier: deposit.nullifier, + secret: deposit.secret, + pathElements: path_elements, + pathIndices: path_index, + }) + + + const proofData = await websnarkUtils.genWitnessAndProve(groth16, input, circuit, proving_key) + const { proof } = websnarkUtils.toSolidityInput(proofData) + + const balanceMixerBefore = await token.balanceOf(mixer.address) + const balanceRelayerBefore = await token.balanceOf(relayer) + const balanceRecieverBefore = await token.balanceOf(toHex(recipient.toString())) + + const ethBalanceOperatorBefore = await web3.eth.getBalance(operator) + const ethBalanceRecieverBefore = await web3.eth.getBalance(toHex(recipient.toString())) + const ethBalanceRelayerBefore = await web3.eth.getBalance(relayer) + let isSpent = await mixer.isSpent(toFixedHex(input.nullifierHash)) + isSpent.should.be.equal(false) + + const args = [ + toFixedHex(input.root), + toFixedHex(input.nullifierHash), + toFixedHex(input.recipient, 20), + toFixedHex(input.relayer, 20), + toFixedHex(input.fee), + toFixedHex(input.refund) + ] + const { logs } = await mixer.withdraw(proof, ...args, { value: refund, from: relayer, gasPrice: '0' }) + + const balanceMixerAfter = await token.balanceOf(mixer.address) + const balanceRelayerAfter = await token.balanceOf(relayer) + const ethBalanceOperatorAfter = await web3.eth.getBalance(operator) + const balanceRecieverAfter = await token.balanceOf(toHex(recipient.toString())) + const ethBalanceRecieverAfter = await web3.eth.getBalance(toHex(recipient.toString())) + const ethBalanceRelayerAfter = await web3.eth.getBalance(relayer) + const feeBN = toBN(fee.toString()) + balanceMixerAfter.should.be.eq.BN(toBN(balanceMixerBefore).sub(toBN(tokenDenomination))) + balanceRelayerAfter.should.be.eq.BN(toBN(balanceRelayerBefore).add(feeBN)) + balanceRecieverAfter.should.be.eq.BN(toBN(balanceRecieverBefore).add(toBN(tokenDenomination).sub(feeBN))) + + ethBalanceOperatorAfter.should.be.eq.BN(toBN(ethBalanceOperatorBefore)) + ethBalanceRecieverAfter.should.be.eq.BN(toBN(ethBalanceRecieverBefore)) + ethBalanceRelayerAfter.should.be.eq.BN(toBN(ethBalanceRelayerBefore)) + + logs[0].event.should.be.equal('Withdrawal') + logs[0].args.nullifierHash.should.be.equal(toFixedHex(input.nullifierHash)) + logs[0].args.relayer.should.be.eq.BN(relayer) + logs[0].args.fee.should.be.eq.BN(feeBN) + isSpent = await mixer.isSpent(toFixedHex(input.nullifierHash)) + isSpent.should.be.equal(true) + }) + it('should reject with wrong refund value', async () => { const deposit = generateDeposit() const user = accounts[4]