2

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

  • 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. – NowsyMe Aug 6 '18 at 10:55
  • 1
    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. – NowsyMe Aug 6 '18 at 11:00
  • 1
    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? – Xavier Leprêtre B9lab Aug 6 '18 at 15:00
  • 1
    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? – Xavier Leprêtre B9lab Aug 6 '18 at 15:01
  • 1
    I made an edit. Look at my "additionally", I think you have further problems. Start anew with a better data structure. – Xavier Leprêtre B9lab Aug 6 '18 at 15:47
0

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. – user19510 Aug 7 '18 at 4:06
  • I don't understand why this then works if you use literal 10 instead of val. – Lauri Peltonen Aug 7 '18 at 6:03
  • 2
    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)." – Jaime Aug 7 '18 at 8:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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