diff --git a/source/swap-erc20/contracts/SwapERC20.sol b/source/swap-erc20/contracts/SwapERC20.sol index 65728ecef..0ddcafc9a 100644 --- a/source/swap-erc20/contracts/SwapERC20.sol +++ b/source/swap-erc20/contracts/SwapERC20.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.23; import "@openzeppelin/contracts/access/Ownable.sol"; import "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; +import "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import "./interfaces/INoReturnERC20.sol"; import "./interfaces/ISwapERC20.sol"; @@ -256,7 +257,7 @@ contract SwapERC20 is ISwapERC20, Ownable, EIP712 { ); // Ensure the signatory is not null - if (signatory == address(0)) revert SignatureInvalid(); + if (signatory == address(0)) revert Unauthorized(); // Ensure the nonce is not yet used and if not mark it used if (!_markNonceAsUsed(signatory, nonce)) revert NonceAlreadyUsed(nonce); @@ -264,7 +265,7 @@ contract SwapERC20 is ISwapERC20, Ownable, EIP712 { // Ensure signatory is authorized to sign if (authorized[signerWallet] != address(0)) { // If one is set by signer wallet, signatory must be authorized - if (signatory != authorized[signerWallet]) revert SignatoryUnauthorized(); + if (signatory != authorized[signerWallet]) revert Unauthorized(); } else { // Otherwise, signatory must be signer wallet if (signatory != signerWallet) revert Unauthorized(); @@ -447,42 +448,32 @@ contract SwapERC20 is ISwapERC20, Ownable, EIP712 { order.s = s; order.senderWallet = senderWallet; - address signatory = ecrecover( - _getOrderHash( - order.nonce, - order.expiry, - order.signerWallet, - order.signerToken, - order.signerAmount, - order.senderWallet, - order.senderToken, - order.senderAmount - ), - order.v, - order.r, - order.s - ); + address signatory = order.signerWallet; + if (authorized[signatory] != address(0)) { + signatory = authorized[signatory]; + } - if (signatory == address(0)) { - errors[errCount] = "SignatureInvalid"; + if ( + !SignatureChecker.isValidSignatureNow( + signatory, + _getOrderHash( + order.nonce, + order.expiry, + order.signerWallet, + order.signerToken, + order.signerAmount, + order.senderWallet, + order.senderToken, + order.senderAmount + ), + abi.encodePacked(r, s, v) + ) + ) { + errors[errCount] = "Unauthorized"; + errCount++; + } else if (nonceUsed(signatory, order.nonce)) { + errors[errCount] = "NonceAlreadyUsed"; errCount++; - } else { - if ( - authorized[order.signerWallet] != address(0) && - signatory != authorized[order.signerWallet] - ) { - errors[errCount] = "SignatoryUnauthorized"; - errCount++; - } else if ( - authorized[order.signerWallet] == address(0) && - signatory != order.signerWallet - ) { - errors[errCount] = "Unauthorized"; - errCount++; - } else if (nonceUsed(signatory, order.nonce)) { - errors[errCount] = "NonceAlreadyUsed"; - errCount++; - } } if (order.expiry < block.timestamp) { @@ -639,35 +630,28 @@ contract SwapERC20 is ISwapERC20, Ownable, EIP712 { // Ensure the expiry is not passed if (expiry <= block.timestamp) revert OrderExpired(); - // Recover the signatory from the hash and signature - (address signatory, ) = ECDSA.tryRecover( - _getOrderHash( - nonce, - expiry, - signerWallet, - signerToken, - signerAmount, - senderWallet, - senderToken, - senderAmount - ), - v, - r, - s - ); - - // Ensure the signatory is not null - if (signatory == address(0)) revert SignatureInvalid(); - - // Ensure signatory is authorized to sign - if (authorized[signerWallet] != address(0)) { - // If one is set by signer wallet, signatory must be authorized - if (signatory != authorized[signerWallet]) revert SignatoryUnauthorized(); - } else { - // Otherwise, signatory must be signer wallet - if (signatory != signerWallet) revert Unauthorized(); + address signatory = signerWallet; + if (authorized[signatory] != address(0)) { + signatory = authorized[signatory]; } + if ( + !SignatureChecker.isValidSignatureNow( + signatory, + _getOrderHash( + nonce, + expiry, + signerWallet, + signerToken, + signerAmount, + senderWallet, + senderToken, + senderAmount + ), + abi.encodePacked(r, s, v) + ) + ) revert Unauthorized(); + // Ensure the nonce is not yet used and if not mark it used if (!_markNonceAsUsed(signatory, nonce)) revert NonceAlreadyUsed(nonce); } diff --git a/source/swap-erc20/contracts/interfaces/ISwapERC20.sol b/source/swap-erc20/contracts/interfaces/ISwapERC20.sol index 2d1861776..b475621b1 100644 --- a/source/swap-erc20/contracts/interfaces/ISwapERC20.sol +++ b/source/swap-erc20/contracts/interfaces/ISwapERC20.sol @@ -37,9 +37,7 @@ interface ISwapERC20 { error MaxTooHigh(); error NonceAlreadyUsed(uint256); error ScaleTooHigh(); - error SignatureInvalid(); error SignatoryInvalid(); - error SignatoryUnauthorized(); error Unauthorized(); function swap( diff --git a/source/swap-erc20/test/SwapERC20.js b/source/swap-erc20/test/SwapERC20.js index 6350e2f88..d38135941 100644 --- a/source/swap-erc20/test/SwapERC20.js +++ b/source/swap-erc20/test/SwapERC20.js @@ -374,7 +374,7 @@ describe('SwapERC20 Unit', () => { await expect( swap.connect(sender).swap(sender.address, ...order) - ).to.be.revertedWith('SignatoryUnauthorized') + ).to.be.revertedWith('Unauthorized') }) it('test when signer not authorized', async () => { @@ -427,15 +427,6 @@ describe('SwapERC20 Unit', () => { .to.be.revertedWith('NonceAlreadyUsed') .withArgs(1) }) - - it('test invalid signature', async () => { - const order = await createSignedOrderERC20({}, signer) - order[7] = '29' // Change "v" of signature - await expect( - swap.connect(sender).swap(sender.address, ...order) - ).to.be.revertedWith('SignatureInvalid') - }) - it('test when signer is zero address', async () => { const order = await createSignedOrderERC20( { @@ -532,7 +523,7 @@ describe('SwapERC20 Unit', () => { order[7] = '29' // Change "v" of signature await expect( swap.connect(sender).swapAnySender(sender.address, ...order) - ).to.be.revertedWith('SignatureInvalid') + ).to.be.revertedWith('Unauthorized') }) }) @@ -583,7 +574,7 @@ describe('SwapERC20 Unit', () => { .withArgs(signer.address, anyone.address) await expect(swap.connect(sender).swapLight(...order)).to.be.revertedWith( - 'SignatoryUnauthorized' + 'Unauthorized' ) }) it('test when expiration has passed', async () => { @@ -603,7 +594,7 @@ describe('SwapERC20 Unit', () => { const order = await createSignedOrderERC20({}, signer) order[7] = '29' // Change "v" of signature await expect(swap.connect(sender).swapLight(...order)).to.be.revertedWith( - 'SignatureInvalid' + 'Unauthorized' ) }) it('test when nonce has already been used', async () => { @@ -766,17 +757,6 @@ describe('SwapERC20 Unit', () => { }) describe('Test check helper', () => { - it('properly detects an invalid signature', async () => { - await setUpAllowances(DEFAULT_AMOUNT, DEFAULT_AMOUNT + SWAP_FEE) - await setUpBalances(DEFAULT_BALANCE, DEFAULT_BALANCE) - const order = await createSignedOrderERC20({}, signer) - order[7] = '29' - const [errCount, messages] = await getErrorInfo(order) - expect(errCount).to.equal(1) - expect(ethers.utils.parseBytes32String(messages[0])).to.equal( - 'SignatureInvalid' - ) - }) it('properly detects an expired order', async () => { await setUpAllowances(DEFAULT_AMOUNT, DEFAULT_AMOUNT + SWAP_FEE) await setUpBalances(DEFAULT_BALANCE, DEFAULT_BALANCE) @@ -792,22 +772,6 @@ describe('SwapERC20 Unit', () => { 'OrderExpired' ) }) - it('properly detects a SignatoryUnauthorized() signature', async () => { - await setUpAllowances(DEFAULT_AMOUNT, DEFAULT_AMOUNT + SWAP_FEE) - await setUpBalances(DEFAULT_BALANCE, DEFAULT_BALANCE) - await swap.connect(signer).authorize(anyone.address) - const order = await createSignedOrderERC20( - { - signer, - }, - signer - ) - const [errCount, messages] = await getErrorInfo(order) - expect(errCount).to.equal(1) - expect(ethers.utils.parseBytes32String(messages[0])).to.equal( - 'SignatoryUnauthorized' - ) - }) it('properly detects an Unauthorized() signature', async () => { await setUpAllowances(DEFAULT_AMOUNT, DEFAULT_AMOUNT + SWAP_FEE) await setUpBalances(DEFAULT_BALANCE, DEFAULT_BALANCE)