duaraghav8 / solium-plugin-security

The Official Security Plugin for Ethlint (formerly Solium)
http://npmjs.com/package/solium-plugin-security
MIT License
44 stars 12 forks source link

Added no-mod-iter-var-for-loop rule #7

Closed nfeignon closed 6 years ago

nfeignon commented 6 years ago

In response to the Augur bug bounty : Prohibit modification of for loop iteration counting variables in the loop body.

This commit identifies the iteration variable in the "init" phase of the for loop, it has to be an AssignmentExpression. It then looks for UpdateExpression or AssignmentExpression in the loop's body.

Let me know if there's anything to fix :)

duaraghav8 commented 6 years ago

@nfeignon awesome! Please go through my review and feel free to discuss anything with me.

Your logic is correct, though I have a doubt. I'm not fully aware of how solidity handles scopes, but please add a test for something like:

contract Test {
    uint i = 2093;

    function foo() {
        for(uint i = 0; i < 10; i++) {
            i += 10;
        }
    }
}

Notice that i is an init value as well as a state variable. Should the i += 10 raise an error? (This depends on whether the i in the statement points to the init value or state var.

Also, the pyramid of doom & comma review comment applies to all your PRs

nfeignon commented 6 years ago

Thanks for the review.

I tested this contract in the Remix IDE. The i declared in the for statement is local to the function foo. So in this case, I think that i += 10 should raise an error.

I've updated the pull request with your requested changes :) Tell me if there's anything else.