Is my Smart Contract Secure and Optimized properly?

Question

how safe is this code and is it possible to optimize it

// SPDX-License-Identifier: AGPL

pragma solidity ^0.8.17;

interface IBEP20 {
    function transfer(address to, uint256 value) external returns (bool);
    function balanceOf(address who) external view returns (uint256);
}


contract TokenDistributerGBR {
    address[] public recipientsList;
    address public owner;
    IBEP20 public token;
    mapping(address => bool) public recipients;

    event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);

    constructor(address _tokenAddress) {
        _tokenAddress = 0xA970cAE9Fa1D7Cca913b7C19DF45BF33d55384A9;
        owner = msg.sender;
        token = IBEP20(_tokenAddress);
    }

    modifier onlyOwner() {
        require(msg.sender == owner, "Only the contract owner can perform this action");
        _;
    }

    function transferOwnership(address newOwner) public onlyOwner {
        require(newOwner != address(0), "New owner cannot be the zero address");
        emit OwnershipTransferred(owner, newOwner);
        owner = newOwner;
    }

    function addRecipient(address recipient) public onlyOwner {
        recipients[recipient] = true;
    }

    function removeRecipient(address recipient) public onlyOwner {
        recipients[recipient] = false;
    }

    function recipientsCount() public view returns (uint256 count) {
        for (uint256 i = 0; i < recipientsList.length; i++) {
            if (recipients[recipientsList[i]]) {
                count++;
            }
        }
    }

    function distributeTokens() public onlyOwner {
    require(token.balanceOf(address(this)) >= 1000000 * recipientsList.length, "Insufficient balance to distribute tokens");

    for (uint256 i = 0; i < recipientsList.length; i++) {
        if (recipientsList[i] != address(0)) {
            require(token.transfer(recipientsList[i], 1000000), "Token transfer failed");
        }
    }
}
}

Answer 1

The code seems to be secure but here are a couple of things you can do to improve the overall efficiency of the smart contract

1. Remove the hardcoded _tokenAddress value from the constructor so it would look like this

constructor(address _tokenAddress) {
        owner = msg.sender;
        token = IBEP20(_tokenAddress);
    }

2. Add a recipientsCount variable to store the number of recipients instead of iterating over the entire recipientsList to count them. This can make the recipientsCount function more efficient.

uint256 public recipientsCount;

function distributeTokens() public onlyOwner {
        require(token.balanceOf(address(this)) >= 1000000 * recipientsCount, "Insufficient balance to distribute tokens");

        for (uint256 i = 0; i < recipientsList.length; i++) {
            if (recipients[recipientsList[i]]) {
                require(token.transfer(recipientsList[i], 1000000), "Token transfer failed");
            }
        }

Additionally, same variable will also be used in below 2 functions from point 3.

3. Modify the addRecipient and removeRecipient functions to check if the recipient is already added or removed before updating the recipients mapping and recipientsCount variable to avoid unnecessary gas costs. The updated code should look like this

function addRecipient(address recipient) public onlyOwner {
        if (!recipients[recipient]) {
            recipients[recipient] = true;
            recipientsCount++;
        }
    }

    function removeRecipient(address recipient) public onlyOwner {
        if (recipients[recipient]) {
            recipients[recipient] = false;
            recipientsCount--;
        }
    }