Solidity Math: What's wrong with the function

Question

What is wrong with the math logic in the function? It shorts the supply of tokens and it distributes much less than expected.

if the contract owns 1000 tokens and the total ether fund size is 30 ether (accumulated by two contributions of 10 ether & 20 ether), then 33,33% of the 1000 tokens should be given to the contributor with a contribution of 10 ether and the rest to the contributor of 20 ether. 


function distribute() external onlyOwner {

    for (uint256 i = 0; i < contributors.length; i++) {
        if (contributed[contributors[i]]) {

            uint256 tokensToDistribute = ((token.balanceOf(environment)).mul(((contributions[contributors[i]].mul(100)).div(funds))).div(100));
            token.transfer(contributors[i], tokensToDistribute);

            tokensDistributed = tokensDistributed.add(tokensToDistribute);

            emit TokensDistributed(contributors[i], environment, token.balanceOf(environment), tokensToDistribute, true);
        }
    }
}

Answer 1

You have only mul and div on the line, so you should try to multiply first before you divide. Because if you divide too early, then you hit 0 and it's game over. Of course, if the numbers are too big, you hit an assert, but this is less likely. So if you rewrite your line, and remove the excessive ( ):

uint256 tokensToDistribute = token.balanceOf(environment).mul(contributions[contributors[i]]).mul(100).div(funds).div(100);

The change is only in ( ) so look in detail and go back to your old line to see how you divided too early.

If this new line works, then check if this one does too:

uint256 tokensToDistribute = token.balanceOf(environment).mul(contributions[contributors[i]]).div(funds);

---

(Edit)

uint envBal = token.balanceOf(environment);
for (uint256 i = 0; i < contributors.length; i++) {
    if (contributed[contributors[i]]) {

        uint256 tokensToDistribute = envBal.mul(contributions[contributors[i]]).mul(100).div(funds).div(100);;
        token.transfer(contributors[i], tokensToDistribute);

        tokensDistributed = tokensDistributed.add(tokensToDistribute);

        emit TokensDistributed(contributors[i], environment, token.balanceOf(environment), tokensToDistribute, true);
    }
}

---

Additionally, doing for (uint256 i = 0; i < contributors.length; i++) { on an unbounded length is a big no-no. Because you will end up using more gas than the block gas limit, which would revert the whole transaction and lock you out of this function.

Review your data structure, and avoid any O(n).

Also, review your data structure and avoid having to do if (contributed[contributors[i]]). You should not need to do that. If there is a contributor, then this contributor has contributed.

Answer 2

Unfortunately, this is not the answer you are looking for.

I am unsure why this happens. It has to do something with number handling, I assume.

Here's a simplified example:

pragma solidity ^0.4.24;

contract Test {

    mapping(uint256 => uint256) contributors;

    function distribute() public returns (uint256) {
        contributors[0] = 10;
        uint256 val = contributors[0];
        return val / 20 * 10;
    }
}

It returns 0. But if you explicitly change val to 10 in the last line it works just fine. Maybe someone else can continue and elaborate on this?