Security and Performance of Solidity Contract

Question

I continue to explore solidity. I wrote a small distributor contract that collects ETH from incoming transactions, records the addresses of incoming transactions with a given condition, and distributes the balance after every 10 transactions. How safe and productive is this contract, what technical comments or additions could you give?

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/utils/math/SafeMath.sol";

contract EthDistributor {
using SafeMath for uint256;

address[] private transactions;
uint256 public transactionSize = 5000000000000000; // 0.005 ETH in wei
uint256 private transactionCount = 0;

event PaymentReceived(address indexed _from, uint256 _value);
event PaymentSplit(uint256 indexed _amount, uint256 indexed _count);

function recordTransaction() public payable {
require(msg.value >= transactionSize, "Transaction value too small");
require(tx.origin == msg.sender, "Proxy contract not allowed");

transactions.push(msg.sender);
transactionCount = transactionCount.add(1);
transactionSize = transactionSize.mul(101).div(100); // increase by 1%

emit PaymentReceived(msg.sender, msg.value);

if (transactionCount == 10) {
uint256 balance = address(this).balance;
uint256 splitAmount = balance.div(transactions.length);
for (uint256 i = 0; i < transactions.length; i++) {
payable(transactions[i]).transfer(splitAmount);
}
transactionCount = 0;
emit PaymentSplit(splitAmount, transactions.length);
}
}
}

Answer 1

There is a few things you should change for security and performance.

First you are vulnerable to areentrancy attacks because the .transfer() function is used inside a loop, then you should use a for loop in reverse to avoid expensive SSTORE operations when removing elements from the transactions array.

Here is your modified contract:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/utils/math/SafeMath.sol";
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

contract EthDistributor is ReentrancyGuard {
    using SafeMath for uint256;

    address[] private transactions;
    uint256 public transactionSize = 5000000000000000; // 0.005 ETH in wei
    uint256 private transactionCount = 0;

    event PaymentReceived(address indexed _from, uint256 _value);
    event PaymentSplit(uint256 indexed _amount, uint256 indexed _count);

    function recordTransaction() public payable nonReentrant {
        require(msg.value >= transactionSize, "Transaction value too small");
        require(tx.origin == msg.sender, "Proxy contract not allowed");

        transactions.push(msg.sender);
        transactionCount = transactionCount.add(1);
        transactionSize = transactionSize.mul(101).div(100); // increase by 1%

        emit PaymentReceived(msg.sender, msg.value);

        if (transactionCount == 10) {
            uint256 balance = address(this).balance;
            uint256 splitAmount = balance.div(transactions.length);
            for (uint256 i = 0; i < transactions.length; i++) {
                payable(transactions[i]).transfer(splitAmount);
            }
            transactionCount = 0;
            emit PaymentSplit(splitAmount, transactions.length);
        }
    }
}