From 13bd330e67f1cf554f1a788d5293044fcc651ec9 Mon Sep 17 00:00:00 2001 From: Alexey Date: Tue, 5 Oct 2021 15:12:39 +0300 Subject: [PATCH] add L1 msg.sender validation --- contracts/Mocks/MockAMB.sol | 1 + contracts/Mocks/MockOmniBridge.sol | 1 + contracts/TornadoPool.sol | 34 +++++++++++++- package.json | 2 + test/full.test.js | 72 ++++++++++++++++++------------ test/utils.js | 45 +++++++++++++++++++ yarn.lock | 29 ++++++++++-- 7 files changed, 150 insertions(+), 34 deletions(-) create mode 100644 test/utils.js diff --git a/contracts/Mocks/MockAMB.sol b/contracts/Mocks/MockAMB.sol index d6b430e..c7356d1 100644 --- a/contracts/Mocks/MockAMB.sol +++ b/contracts/Mocks/MockAMB.sol @@ -26,5 +26,6 @@ contract MockAMB is IAMB { function execute(address _who, bytes calldata _calldata) external returns (bool success, bytes memory result) { (success, result) = _who.call(_calldata); + require(success, string(result)); } } diff --git a/contracts/Mocks/MockOmniBridge.sol b/contracts/Mocks/MockOmniBridge.sol index d90817e..17245bf 100644 --- a/contracts/Mocks/MockOmniBridge.sol +++ b/contracts/Mocks/MockOmniBridge.sol @@ -16,6 +16,7 @@ contract MockOmniBridge is IOmniBridge { function execute(address _who, bytes calldata _calldata) external returns (bool success, bytes memory result) { (success, result) = _who.call(_calldata); + require(success, string(result)); } event OnTokenTransfer(address contr, address from, address receiver, uint256 value, bytes data); diff --git a/contracts/TornadoPool.sol b/contracts/TornadoPool.sol index a36fec3..2ed5ea3 100644 --- a/contracts/TornadoPool.sol +++ b/contracts/TornadoPool.sol @@ -14,6 +14,7 @@ pragma solidity ^0.7.0; pragma experimental ABIEncoderV2; import "@openzeppelin/contracts/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts/contracts/cryptography/ECDSA.sol"; import "./MerkleTreeWithHistory.sol"; interface IERC6777 is IERC20 { @@ -41,6 +42,8 @@ interface IERC20Receiver { contract TornadoPool is MerkleTreeWithHistory, IERC20Receiver { int256 public constant MAX_EXT_AMOUNT = 2**248; uint256 public constant MAX_FEE = 2**248; + bytes32 public constant ACCOUNT_TYPEHASH = keccak256("TornadoAccount(address owner,bytes publicKey)"); + uint256 public immutable L1_CHAIN_ID; IVerifier public immutable verifier2; IVerifier public immutable verifier16; @@ -91,13 +94,15 @@ contract TornadoPool is MerkleTreeWithHistory, IERC20Receiver { address _hasher, IERC6777 _token, address _omniBridge, - address _l1Unwrapper + address _l1Unwrapper, + uint256 _l1ChainId ) MerkleTreeWithHistory(_levels, _hasher) { verifier2 = _verifier2; verifier16 = _verifier16; token = _token; omniBridge = _omniBridge; l1Unwrapper = _l1Unwrapper; + L1_CHAIN_ID = _l1ChainId; } function transact(Proof memory _args, ExtData memory _extData) public { @@ -227,7 +232,11 @@ contract TornadoPool is MerkleTreeWithHistory, IERC20Receiver { uint256 _amount, bytes calldata _data ) external override { - (Account memory _account, Proof memory _args, ExtData memory _extData) = abi.decode(_data, (Account, Proof, ExtData)); + (Account memory _account, Proof memory _args, ExtData memory _extData, bytes memory _signature) = abi.decode( + _data, + (Account, Proof, ExtData, bytes) + ); + require(isValidSignature(_account, _signature), "Invalid account signature"); require(_token == token, "provided token is not supported"); require(msg.sender == omniBridge, "only omni bridge"); require(_amount == uint256(_extData.extAmount), "amount from bridge is incorrect"); @@ -240,4 +249,25 @@ contract TornadoPool is MerkleTreeWithHistory, IERC20Receiver { } _transact(_args, _extData); } + + function isValidSignature(Account memory _account, bytes memory _signature) public view returns (bool) { + bytes32 hashStruct = keccak256(abi.encode(ACCOUNT_TYPEHASH, _account.owner, keccak256(_account.publicKey))); + bytes32 hash = keccak256(abi.encodePacked(uint16(0x1901), domainSeparator(), hashStruct)); + + address signer = ECDSA.recover(hash, _signature); + return signer == _account.owner; + } + + function domainSeparator() public view returns (bytes32) { + return + keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256(bytes("TornadoPool")), + keccak256(bytes("1")), // Version + L1_CHAIN_ID, + address(this) + ) + ); + } } diff --git a/package.json b/package.json index aac14eb..0eb482a 100644 --- a/package.json +++ b/package.json @@ -22,6 +22,7 @@ "author": "", "license": "ISC", "dependencies": { + "@metamask/eth-sig-util": "^4.0.0", "@nomiclabs/hardhat-ethers": "^2.0.2", "@nomiclabs/hardhat-waffle": "^2.0.1", "@openzeppelin/contracts": "git+https://github.com/tornadocash/openzeppelin-contracts.git#6e46aa6946a7f215e7604169ddf46e1aebea850f", @@ -36,6 +37,7 @@ "dotenv": "^10.0.0", "eth-sig-util": "^3.0.1", "ethereum-waffle": "^3.2.0", + "ethereumjs-util": "^7.1.2", "ethers": "^5.0.0", "ffiasm": "^0.1.3", "ffjavascript": "^0.2.36", diff --git a/test/full.test.js b/test/full.test.js index aa78457..0b55991 100644 --- a/test/full.test.js +++ b/test/full.test.js @@ -4,11 +4,17 @@ const { loadFixture } = waffle const { expect } = require('chai') const { utils } = ethers +const { toBuffer } = require('ethereumjs-util') +const { signTypedData, SignTypedDataVersion } = require('@metamask/eth-sig-util') + const Utxo = require('../src/utxo') const { transaction, registerAndTransact, prepareTransaction } = require('../src/index') const { Keypair } = require('../src/keypair') +const { EIP721Params, encodeDataForBridge } = require('./utils') const MERKLE_TREE_HEIGHT = 5 +const L1ChainId = 1 +const SENDER_PRIVATE_KEY = '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80' describe('TornadoPool', function () { this.timeout(20000) @@ -26,14 +32,14 @@ describe('TornadoPool', function () { const verifier16 = await deploy('Verifier16') const hasher = await deploy('Hasher') - const token = await deploy('PermittableToken', 'Wrapped ETH', 'WETH', 18, 1) + const token = await deploy('PermittableToken', 'Wrapped ETH', 'WETH', 18, L1ChainId) await token.mint(sender.address, utils.parseEther('10000')) - const amb = await deploy('MockAMB', gov.address, 1) + const amb = await deploy('MockAMB', gov.address, L1ChainId) const omniBridge = await deploy('MockOmniBridge', amb.address) /** @type {TornadoPool} */ - const tornadoPool = await deploy( + const tornadoPoolImpl = await deploy( 'TornadoPool', verifier2.address, verifier16.address, @@ -42,43 +48,38 @@ describe('TornadoPool', function () { token.address, omniBridge.address, l1Unwrapper.address, + L1ChainId, ) - await tornadoPool.initialize() + await tornadoPoolImpl.initialize() // not necessary - await token.approve(tornadoPool.address, utils.parseEther('10000')) - return { tornadoPool, token, omniBridge, amb } - } - - async function fixtureUpgradeable() { - const { tornadoPool, omniBridge, amb } = await loadFixture(fixture) - const [, gov] = await ethers.getSigners() const proxy = await deploy( 'CrossChainUpgradeableProxy', - tornadoPool.address, + tornadoPoolImpl.address, gov.address, [], amb.address, - 1, + L1ChainId, ) - /** @type {TornadoPool} */ const TornadoPool = await ethers.getContractFactory('TornadoPool') - const tornadoPoolProxied = TornadoPool.attach(proxy.address) - await tornadoPoolProxied.initialize() + const tornadoPool = TornadoPool.attach(proxy.address) + await tornadoPool.initialize() - return { tornadoPool: tornadoPoolProxied, proxy, gov, omniBridge, amb } + await token.approve(tornadoPool.address, utils.parseEther('10000')) + + return { tornadoPool, token, proxy, omniBridge, amb, gov } } describe('Upgradeability tests', () => { it('admin should be gov', async () => { - const { proxy, amb, gov } = await loadFixture(fixtureUpgradeable) + const { proxy, amb, gov } = await loadFixture(fixture) const { data } = await proxy.populateTransaction.admin() const { result } = await amb.callStatic.execute(proxy.address, data) expect('0x' + result.slice(26)).to.be.equal(gov.address.toLowerCase()) }) it('non admin cannot call', async () => { - const { proxy } = await loadFixture(fixtureUpgradeable) + const { proxy } = await loadFixture(fixture) await expect(proxy.admin()).to.be.revertedWith( "Transaction reverted: function selector was not recognized and there's no fallback function", ) @@ -202,24 +203,38 @@ describe('TornadoPool', function () { it('should deposit from L1 and withdraw to L1', async function () { const { tornadoPool, token, omniBridge } = await loadFixture(fixture) - // console.log('tornadoPool', tornadoPool.interface) + const owner = (await ethers.getSigners())[0].address + const aliceKeypair = new Keypair() // contains private and public keys // Alice deposits into tornado pool const aliceDepositAmount = 1e7 - const aliceDepositUtxo = new Utxo({ amount: aliceDepositAmount }) + const aliceDepositUtxo = new Utxo({ amount: aliceDepositAmount, keypair: aliceKeypair }) const { args, extData } = await prepareTransaction({ tornadoPool, outputs: [aliceDepositUtxo], }) - const transactTx = await tornadoPool.populateTransaction.registerAndTransact( - { - owner: '0x0000000000000000000000000000000000000000', - publicKey: [], + + const signature = signTypedData({ + privateKey: toBuffer(SENDER_PRIVATE_KEY), + data: EIP721Params({ + chainId: L1ChainId, + verifyingContract: tornadoPool.address, + owner, + publicKey: aliceKeypair.address(), + }), + version: SignTypedDataVersion.V4, + }) + + const onTokenBridgedData = encodeDataForBridge({ + account: { + owner, + publicKey: aliceKeypair.address(), }, - args, + proof: args, extData, - ) - const onTokenBridgedData = '0x' + transactTx.data.slice(10) + signature, + }) + const onTokenBridgedTx = await tornadoPool.populateTransaction.onTokenBridged( token.address, aliceDepositUtxo.amount, @@ -230,7 +245,6 @@ describe('TornadoPool', function () { await omniBridge.execute(tornadoPool.address, onTokenBridgedTx.data) // withdraws a part of his funds from the shielded pool - const aliceKeypair = new Keypair() // contains private and public keys const aliceWithdrawAmount = 2e6 const recipient = '0xDeaD00000000000000000000000000000000BEEf' const aliceChangeUtxo = new Utxo({ diff --git a/test/utils.js b/test/utils.js new file mode 100644 index 0000000..8d98de4 --- /dev/null +++ b/test/utils.js @@ -0,0 +1,45 @@ +const { ethers } = require('hardhat') + +const abi = new ethers.utils.AbiCoder() + +function encodeDataForBridge({ account, proof, extData, signature }) { + return abi.encode( + [ + 'tuple(address owner,bytes publicKey)', + 'tuple(bytes proof,bytes32 root,bytes32[] inputNullifiers,bytes32[2] outputCommitments,uint256 publicAmount,bytes32 extDataHash)', + 'tuple(address recipient,int256 extAmount,address relayer,uint256 fee,bytes encryptedOutput1,bytes encryptedOutput2,bool isL1Withdrawal)', + 'bytes', + ], + [account, proof, extData, signature], + ) +} + +function EIP721Params({ chainId, verifyingContract, owner, publicKey }) { + return { + types: { + EIP712Domain: [ + { name: 'name', type: 'string' }, + { name: 'version', type: 'string' }, + { name: 'chainId', type: 'uint256' }, + { name: 'verifyingContract', type: 'address' }, + ], + TornadoAccount: [ + { name: 'owner', type: 'address' }, + { name: 'publicKey', type: 'bytes' }, + ], + }, + primaryType: 'TornadoAccount', + domain: { + name: 'TornadoPool', + version: '1', + chainId, + verifyingContract, + }, + message: { + owner, + publicKey, + }, + } +} + +module.exports = { encodeDataForBridge, EIP721Params } diff --git a/yarn.lock b/yarn.lock index 30abbc6..441a64e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -656,6 +656,17 @@ fastfile "0.0.19" ffjavascript "^0.2.30" +"@metamask/eth-sig-util@^4.0.0": + version "4.0.0" + resolved "https://registry.yarnpkg.com/@metamask/eth-sig-util/-/eth-sig-util-4.0.0.tgz#11553ba06de0d1352332c1bde28c8edd00e0dcf6" + integrity sha512-LczOjjxY4A7XYloxzyxJIHONELmUxVZncpOLoClpEcTiebiVdM46KRPYXGuULro9oNNR2xdVx3yoKiQjdfWmoA== + dependencies: + ethereumjs-abi "^0.6.8" + ethereumjs-util "^6.2.1" + ethjs-util "^0.1.6" + tweetnacl "^1.0.3" + tweetnacl-util "^0.15.1" + "@nomiclabs/hardhat-ethers@^2.0.2": version "2.0.2" resolved "https://registry.yarnpkg.com/@nomiclabs/hardhat-ethers/-/hardhat-ethers-2.0.2.tgz#c472abcba0c5185aaa4ad4070146e95213c68511" @@ -3723,7 +3734,7 @@ ethereumjs-tx@^1.1.1, ethereumjs-tx@^1.2.0, ethereumjs-tx@^1.2.2, ethereumjs-tx@ ethereum-common "^0.0.18" ethereumjs-util "^5.0.0" -ethereumjs-util@6.2.1, ethereumjs-util@^6.0.0, ethereumjs-util@^6.1.0, ethereumjs-util@^6.2.0: +ethereumjs-util@6.2.1, ethereumjs-util@^6.0.0, ethereumjs-util@^6.1.0, ethereumjs-util@^6.2.0, ethereumjs-util@^6.2.1: version "6.2.1" resolved "https://registry.yarnpkg.com/ethereumjs-util/-/ethereumjs-util-6.2.1.tgz#fcb4e4dd5ceacb9d2305426ab1a5cd93e3163b69" integrity sha512-W2Ktez4L01Vexijrm5EB6w7dg4n/TgpoYU4avuT5T3Vmnw/eCRtiBrJfQYS/DCSvDIOLn2k57GcHdeBcgVxAqw== @@ -3772,6 +3783,18 @@ ethereumjs-util@^7.0.10, ethereumjs-util@^7.0.2, ethereumjs-util@^7.0.7, ethereu ethjs-util "0.1.6" rlp "^2.2.4" +ethereumjs-util@^7.1.2: + version "7.1.2" + resolved "https://registry.yarnpkg.com/ethereumjs-util/-/ethereumjs-util-7.1.2.tgz#cfd79a9a3f5cdc042d1abf29964de9caf10ec238" + integrity sha512-xCV3PTAhW8Q2k88XZn9VcO4OrjpeXAlDm5LQTaOLp81SjNSSY6+MwuGXrx6vafOMheWSmZGxIXUbue5e9UvUBw== + dependencies: + "@types/bn.js" "^5.1.0" + bn.js "^5.1.2" + create-hash "^1.1.2" + ethereum-cryptography "^0.1.3" + ethjs-util "0.1.6" + rlp "^2.2.4" + ethereumjs-vm@4.2.0: version "4.2.0" resolved "https://registry.yarnpkg.com/ethereumjs-vm/-/ethereumjs-vm-4.2.0.tgz#e885e861424e373dbc556278f7259ff3fca5edab" @@ -3869,7 +3892,7 @@ ethjs-unit@0.1.6: bn.js "4.11.6" number-to-bn "1.7.0" -ethjs-util@0.1.6, ethjs-util@^0.1.3: +ethjs-util@0.1.6, ethjs-util@^0.1.3, ethjs-util@^0.1.6: version "0.1.6" resolved "https://registry.yarnpkg.com/ethjs-util/-/ethjs-util-0.1.6.tgz#f308b62f185f9fe6237132fb2a9818866a5cd536" integrity sha512-CUnVOQq7gSpDHZVVrQW8ExxUETWrnrvXYvYz55wOU8Uj4VCgw56XC2B/fVqQN+f7gmrnRHSLVnFAwsCuNwji8w== @@ -8642,7 +8665,7 @@ tunnel-agent@^0.6.0: dependencies: safe-buffer "^5.0.1" -tweetnacl-util@^0.15.0: +tweetnacl-util@^0.15.0, tweetnacl-util@^0.15.1: version "0.15.1" resolved "https://registry.yarnpkg.com/tweetnacl-util/-/tweetnacl-util-0.15.1.tgz#b80fcdb5c97bcc508be18c44a4be50f022eea00b" integrity sha512-RKJBIj8lySrShN4w6i/BonWp2Z/uxwC3h4y7xsRrpP59ZboCd0GpEVsOnMDYLMmKBpYhb5TgHzZXy7wTfYFBRw==