# Solidity Math: What's wrong with the function

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

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

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

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.

• both lines work in the sense of calculating some sort of return. However, the wrong number is still being distributed. In a scenario where the contract owns 1000 tokens, and 60 ETH in funds (accumulated through three contributions of 10, 20 & 30 ETH) - approximately 33% of tokens are being leftover/not distributed to anyone. Aug 6, 2018 at 10:55
• I believe it has something to do with the token.balanceOf(environment) being reduced for every iteration. However, I tried to approach the problem by creating a new variable tokensAvailable = tokensAvailable.add(token.balanceOf(environment).add(tokensDistributed) which I believe would work BUT the tokensAvailable variable seems to equal 0 (it can't store the token.balanceOf(environment) value. Aug 6, 2018 at 11:00
• In your example, 10 Eth should receive 166 tokens, 20 Eth should receive 333 tokens, and 30 Eth should receive 500 tokens. Leaving 1 undisbursed. Right? Aug 6, 2018 at 15:00
• I see another error in your line. At each iteration, you use `token.balanceOf(environment)`. But I expect this balance to go down after each iteration because of the `.transfer`. Have you tried saving this balance before the loop? Aug 6, 2018 at 15:01
• I made an edit. Look at my "additionally", I think you have further problems. Start anew with a better data structure. Aug 6, 2018 at 15:47

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?

• `val == 10`, so `val / 20`, which does integer math, is `0`. `0 * 10` is still `0`. Multiply first to avoid this: `val * 10 / 20`, but always watch out for integer overflows. Aug 7, 2018 at 4:06
• I don't understand why this then works if you use literal 10 instead of val. Aug 7, 2018 at 6:03
• The reason why this is different whe you use literals is because: "Division always truncates (it is just compiled to the DIV opcode of the EVM), but it does not truncate if both operators are literals (or literal expressions)." Aug 7, 2018 at 8:36