2

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);
}
}
}

1 Answer 1

2

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);
        }
    }
}
2
  • according to my idea, the addresses should not be cleared, the list of addresses should only increase and the distribution should be done on an increasing number of addresses Apr 6, 2023 at 5:21
  • Alright, I modified my answer and the code.
    – Saxtheowl
    Apr 6, 2023 at 5:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.