diff --git a/contracts/Batcher.sol b/contracts/Batcher.sol index 65ba691..4ca099f 100644 --- a/contracts/Batcher.sol +++ b/contracts/Batcher.sol @@ -1,38 +1,50 @@ +// SPDX-License-Identifier: Apache-2.0 pragma solidity 0.8.20; + import '@openzeppelin/contracts/access/Ownable2Step.sol'; +import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol'; -// SPDX-License-Identifier: Apache-2.0 +error EmptyRecipientsList(); +error UnequalRecipientsAndValues(); +error TooManyRecipients(uint256 provided, uint256 limit); +error TokenTransferFailed(address token, address from, address to, uint256 amount); -/** - * - * Batcher - * ======= - * - * Contract that can take a batch of transfers, presented in the form of a recipients array and a values array, and - * funnel off those funds to the correct accounts in a single transaction. This is useful for saving on gas when a - * bunch of funds need to be transferred to different accounts. - * - * If more ETH is sent to `batch` than it is instructed to transfer, then the entire transaction will revert - * If any tokens are accidentally transferred to this account, contact the contract owner in order to recover them. - * - */ +/// @title Batcher - Batch transfer contract for ETH and ERC20 tokens +/// @notice Allows batch transfers of ETH and ERC20 tokens to multiple recipients in a single transaction +/// @dev Implements reentrancy protection and configurable gas limits for transfers contract Batcher is Ownable2Step { + using SafeERC20 for IERC20; + event BatchTransfer(address sender, address recipient, uint256 value); event TransferGasLimitChange( uint256 prevTransferGasLimit, uint256 newTransferGasLimit ); + event BatchTransferLimitChange( + uint256 prevBatchTransferLimit, + uint256 newBatchTransferLimit + ); uint256 public lockCounter; uint256 public transferGasLimit; + uint256 public batchTransferLimit; - constructor(uint256 _transferGasLimit) Ownable(msg.sender) { + /// @notice Contract constructor + /// @param _transferGasLimit Gas limit for individual transfers + /// @param _batchTransferLimit Maximum number of transfers allowed in a batch + /// @dev Sets initial values for transfer limits and initializes the reentrancy guard + constructor(uint256 _transferGasLimit, uint256 _batchTransferLimit) Ownable(msg.sender) { lockCounter = 1; transferGasLimit = _transferGasLimit; + batchTransferLimit = _batchTransferLimit; emit TransferGasLimitChange(0, transferGasLimit); + emit BatchTransferLimitChange(0, batchTransferLimit); } + /// @notice Prevents reentrancy attacks + /// @dev Increments a counter before execution and checks it hasn't changed after modifier lockCall() { lockCounter++; uint256 localCounter = lockCounter; @@ -40,13 +52,11 @@ contract Batcher is Ownable2Step { require(localCounter == lockCounter, 'Reentrancy attempt detected'); } - /** - * Transfer funds in a batch to each of recipients - * @param recipients The list of recipients to send to - * @param values The list of values to send to recipients. - * The recipient with index i in recipients array will be sent values[i]. - * Thus, recipients and values must be the same length - */ + /// @notice Batch transfer ETH to multiple recipients + /// @param recipients The list of recipients to send to + /// @param values The list of values to send to recipients. + /// @dev Total value sent must match msg.value exactly + /// @dev Reverts if any transfer fails or if arrays are mismatched function batch(address[] calldata recipients, uint256[] calldata values) external payable @@ -78,12 +88,33 @@ contract Batcher is Ownable2Step { require(totalSent == msg.value, 'Total sent out must equal total received'); } - /** - * Recovery function for the contract owner to recover any ERC20 tokens or ETH that may get lost in the control of this contract. - * @param to The recipient to send to - * @param value The ETH value to send with the call - * @param data The data to send along with the call - */ + /// @notice Batch transfer ERC20 tokens from sender to multiple recipients + /// @param token Address of the ERC20 token contract + /// @param recipients Array of recipient addresses + /// @param amounts Array of token amounts to transfer to each recipient + /// @dev Requires prior approval for token spending + /// @dev Uses SafeERC20 for secure token transfers + function batchTransferFrom( + address token, + address[] calldata recipients, + uint256[] calldata amounts + ) external lockCall { + if (recipients.length == 0) revert EmptyRecipientsList(); + if (recipients.length != amounts.length) revert UnequalRecipientsAndValues(); + if (recipients.length > batchTransferLimit) revert TooManyRecipients(recipients.length, batchTransferLimit); + + IERC20 safeToken = IERC20(token); + for (uint16 i = 0; i < recipients.length; i++) { + safeToken.safeTransferFrom(msg.sender, recipients[i], amounts[i]); + } + } + + /// @notice Recover any ETH or tokens accidentally sent to the contract + /// @param to Destination address for recovery + /// @param value Amount of ETH to send + /// @param data Calldata for the recovery transaction + /// @return bytes The return data from the recovery call + /// @dev Only callable by contract owner function recover( address to, uint256 value, @@ -94,12 +125,10 @@ contract Batcher is Ownable2Step { return returnData; } - /** - * Change the gas limit that is sent along with batched transfers. - * This is intended to protect against any EVM level changes that would require - * a new amount of gas for an internal send to complete. - * @param newTransferGasLimit The new gas limit to send along with batched transfers - */ + /// @notice Update the gas limit for individual transfers + /// @param newTransferGasLimit New gas limit value + /// @dev Minimum value of 2300 required for basic ETH transfers + /// @dev Only callable by contract owner function changeTransferGasLimit(uint256 newTransferGasLimit) external onlyOwner @@ -109,6 +138,19 @@ contract Batcher is Ownable2Step { transferGasLimit = newTransferGasLimit; } + /// @notice Update the maximum number of transfers allowed in a batch + /// @param newBatchTransferLimit New maximum batch size + /// @dev Must be greater than zero + /// @dev Only callable by contract owner + function changeBatchTransferLimit(uint256 newBatchTransferLimit) + external + onlyOwner + { + require(newBatchTransferLimit > 0, 'Batch transfer limit too low'); + emit BatchTransferLimitChange(batchTransferLimit, newBatchTransferLimit); + batchTransferLimit = newBatchTransferLimit; + } + fallback() external payable { revert('Invalid fallback'); } diff --git a/test/batcher.js b/test/batcher.js index 6bf04b8..4e8907f 100644 --- a/test/batcher.js +++ b/test/batcher.js @@ -41,6 +41,27 @@ const createRandIntArr = (len) => { return arr; }; +const assertCustomError = async (promise, expectedErrorName) => { + let succeeded = false; + try { + await promise; + succeeded = true; + } catch (err) { + // Custom errors are reported in the form: + // "VM Exception while processing transaction: reverted with custom error 'ErrorName(param1,param2)'" + assert( + err.message.includes(`custom error`) && + err.message.includes(`${expectedErrorName}`), + `Expected custom error ${expectedErrorName} but got: ${err.message}` + ); + } + if (succeeded) { + assert.fail( + `Expected custom error ${expectedErrorName} but transaction succeeded` + ); + } +}; + const assertVMException = async (promise, expectedExceptionMsg) => { let badSucceed = false; try { @@ -80,7 +101,7 @@ describe('Batcher', () => { sender = accounts[0]; batcherOwner = accounts[8]; - batcherInstance = await Batcher.new(21000, { from: batcherOwner }); + batcherInstance = await Batcher.new(21000, 256, { from: batcherOwner }); reentryInstance = await Reentry.new(batcherInstance.address); failInstance = await Fail.new(); gasGuzzlerInstance = await GasGuzzler.new(); @@ -664,6 +685,339 @@ describe('Batcher', () => { }); }); + describe('Batch Transfer Limit', () => { + it('Successfully changes batch transfer limit', async () => { + const newLimit = 100; + const tx = await batcherInstance.changeBatchTransferLimit(newLimit, { + from: batcherOwner + }); + + const { + logs: [ + { + args: { prevBatchTransferLimit, newBatchTransferLimit } + } + ] + } = tx; + + assert.strictEqual( + prevBatchTransferLimit.toNumber(), + 256, + 'Previous limit incorrect' + ); + assert.strictEqual( + newBatchTransferLimit.toNumber(), + newLimit, + 'New limit incorrect' + ); + + const currentLimit = await batcherInstance.batchTransferLimit(); + assert.strictEqual( + currentLimit.toNumber(), + newLimit, + 'Limit not updated correctly' + ); + }); + + it('Fails to set batch transfer limit to zero', async () => { + await assertVMException( + batcherInstance.changeBatchTransferLimit(0, { from: batcherOwner }), + 'Batch transfer limit too low' + ); + }); + + it('Fails when non-owner tries to change batch transfer limit', async () => { + await truffleAssert.reverts( + batcherInstance.changeBatchTransferLimit(100, { from: accounts[1] }) + ); + }); + }); + + describe('Batch ERC20 Token Transfers', () => { + let tokenContract; + let totalSupply; + let tokenContractOwner; + + beforeEach(async () => { + tokenContractOwner = accounts[9]; + tokenContract = await FixedSupplyToken.new({ + from: tokenContractOwner + }); + totalSupply = await tokenContract.totalSupply(); + }); + + const checkBalance = async (address, expectedAmt) => { + const balance = await tokenContract.balanceOf(address); + assert.strictEqual( + balance.toString(), + expectedAmt.toString(), + `Token balance of ${address} was ${balance.toString()} when ${expectedAmt.toString()} was expected` + ); + }; + + it('Successfully transfers tokens to multiple recipients', async () => { + const sender = accounts[1]; + const recipients = [accounts[2], accounts[3], accounts[4]]; + const amounts = [100, 200, 300]; + + // Transfer tokens to sender + await tokenContract.transfer(sender, 1000, { + from: tokenContractOwner + }); + await checkBalance(sender, 1000); + + // Approve batcher to spend tokens + await tokenContract.approve(batcherInstance.address, 1000, { + from: sender + }); + + // Execute batch transfer + await batcherInstance.batchTransferFrom( + tokenContract.address, + recipients, + amounts, + { from: sender } + ); + + // Verify balances + await checkBalance(sender, 400); // 1000 - (100 + 200 + 300) + await checkBalance(recipients[0], 100); + await checkBalance(recipients[1], 200); + await checkBalance(recipients[2], 300); + }); + + it('Fails when sender has insufficient balance', async () => { + const sender = accounts[1]; + const recipients = [accounts[2], accounts[3]]; + const amounts = [500, 600]; + + // Transfer fewer tokens than needed to sender + await tokenContract.transfer(sender, 500, { from: tokenContractOwner }); + await tokenContract.approve(batcherInstance.address, 1100, { + from: sender + }); + + await checkBalance(sender, 500); + await checkBalance(recipients[0], 0); + await checkBalance(recipients[1], 0); + + await assertCustomError( + batcherInstance.batchTransferFrom( + tokenContract.address, + recipients, + amounts, + { from: sender } + ), + 'SafeERC20FailedOperation' + ); + + // balance does not change + await checkBalance(sender, 500); + await checkBalance(recipients[0], 0); + await checkBalance(recipients[1], 0); + }); + + it('Fails when sender has not approved enough tokens', async () => { + const sender = accounts[1]; + const recipients = [accounts[2], accounts[3]]; + const amounts = [500, 600]; + + await tokenContract.transfer(sender, 1100, { + from: tokenContractOwner + }); + await tokenContract.approve(batcherInstance.address, 500, { + from: sender + }); + + await checkBalance(sender, 1100); + await checkBalance(recipients[0], 0); + await checkBalance(recipients[1], 0); + + await assertCustomError( + batcherInstance.batchTransferFrom( + tokenContract.address, + recipients, + amounts, + { from: sender } + ), + 'SafeERC20FailedOperation' + ); + + // balance does not change + await checkBalance(sender, 1100); + await checkBalance(recipients[0], 0); + await checkBalance(recipients[1], 0); + }); + + it('Fails with empty recipients array', async () => { + const sender = accounts[1]; + await assertCustomError( + batcherInstance.batchTransferFrom(tokenContract.address, [], [], { + from: sender + }), + 'EmptyRecipientsList' + ); + }); + + it('Fails with mismatched recipients and amounts arrays', async () => { + const sender = accounts[1]; + const recipients = [accounts[2], accounts[3]]; + const amounts = [100]; + + await assertCustomError( + batcherInstance.batchTransferFrom( + tokenContract.address, + recipients, + amounts, + { from: sender } + ), + 'UnequalRecipientsAndValues' + ); + }); + + it('Successfully transfers tokens that do not return bool (like USDT)', async () => { + // Deploy USDT mock + const tetherTokenContract = await Tether.new( + '1000000', + 'USDT', + 'USDT', + 6, + { + from: tokenContractOwner + } + ); + + const sender = accounts[1]; + const recipients = [accounts[2], accounts[3], accounts[4]]; + const amounts = [100, 200, 300]; + const totalAmount = amounts.reduce((a, b) => a + b, 0); + // Transfer tokens to sender + await tetherTokenContract.transfer(sender, 1000, { + from: tokenContractOwner + }); + + // Check initial balances + const senderInitialBalance = await tetherTokenContract.balanceOf.call( + sender + ); + assert.strictEqual( + senderInitialBalance.toString(), + '1000', + 'Incorrect sender initial balance' + ); + + // Approve batcher to spend tokens + await tetherTokenContract.approve(batcherInstance.address, 1000, { + from: sender + }); + + // Execute batch transfer + await batcherInstance.batchTransferFrom( + tetherTokenContract.address, + recipients, + amounts, + { from: sender } + ); + + // Verify final balances + const senderFinalBalance = await tetherTokenContract.balanceOf.call( + sender + ); + assert.strictEqual( + senderFinalBalance.toString(), + (1000 - totalAmount).toString(), + 'Incorrect sender final balance' + ); + + // Check each recipient's balance + for (let i = 0; i < recipients.length; i++) { + const recipientBalance = await tetherTokenContract.balanceOf.call( + recipients[i] + ); + assert.strictEqual( + recipientBalance.toString(), + amounts[i].toString(), + `Incorrect recipient[${i}] balance` + ); + } + }); + + it('Fails correctly when transferring non-bool-returning tokens with insufficient balance', async () => { + // Deploy USDT mock + const tetherTokenContract = await Tether.new( + '1000000', + 'USDT', + 'USDT', + 6, + { + from: tokenContractOwner + } + ); + + const sender = accounts[1]; + const recipients = [accounts[2], accounts[3]]; + const amounts = [500, 600]; // Total 1100, more than sender will have + + // Transfer only 500 tokens to sender (less than needed) + await tetherTokenContract.transfer(sender, 500, { + from: tokenContractOwner + }); + + // Approve batcher to spend tokens + await tetherTokenContract.approve(batcherInstance.address, 1100, { + from: sender + }); + + // Verify initial balances + const senderInitialBalance = await tetherTokenContract.balanceOf.call( + sender + ); + assert.strictEqual( + senderInitialBalance.toString(), + '500', + 'Incorrect sender initial balance' + ); + + // Attempt batch transfer - should fail + try { + await assertCustomError( + batcherInstance.batchTransferFrom( + tetherTokenContract.address, + recipients, + amounts, + { from: sender } + ), + 'TokenTransferFailed' + ); + } catch (err) { + assert( + err.message.includes( + `VM Exception while processing transaction: reverted with panic code 0x1 (Assertion error)` + ) + ); + } + + // Verify balances remained unchanged + const senderFinalBalance = await tetherTokenContract.balanceOf.call( + sender + ); + assert.strictEqual( + senderFinalBalance.toString(), + '500', + 'Sender balance should be unchanged' + ); + + for (const recipient of recipients) { + const balance = await tetherTokenContract.balanceOf.call(recipient); + assert.strictEqual( + balance.toString(), + '0', + 'Recipient should not have received any tokens' + ); + } + }); + }); + describe('Using recover for ERC20 Tokens', () => { let tokenContract; let totalSupply;