Add rule to prohibit calls of .send

[TristanH]

In response to the Augur Bug Bounty: Prohibit use of send

Let me know if there are any fixes/improvements needed :)

Also, currently looking at how to not detect e.g user-defined classes with send functions, but I'm not sure if there's a way to detect that the type of a callee (in the test's case msg.sender) is an address. Any ideas?

[duaraghav8]

please change description to [RULE] no-send: Rejections (this is convention throughout Solium)

[duaraghav8]

remove fixable: 'code' - this property is set only if the rule supplies at least 1 fix.

[TristanH]

thanks, good catch :) I had a fixable in before (changing send to transfer) but I removed it

[duaraghav8]

I thought that must've been the case (has happened with me too!)

[TristanH]

@duaraghav8 I did some digging and it appears there is no way in solparse to detect that the type of an Identifier (in the test example's case msg.sender) is address. Is this true?

If so, some options:

• Add the ability to check the type of an arbitrary Identifier to solparse (not sure how involved this will be...)
• Merge this rule anyways and ignore the largely rare case of folks writing their own function called specifically send

[duaraghav8]

@TristanH That's right, I've had the same thought before of the rare case where user might have defined a send().

• Deducing a type from given identifier is beyond the scope of solparse (since its only responsible for parsing code into AST. So it doesn't know, for eg, where the object foo's bar property is coming from. That is a job more suited for a Symbol Table (make info about all declarations & definitions accessible to each rule Ethlint#83)
• I had put a lot of thought into it and decided to leave solving for this extremely rare case. There are techniques but the time investment is not worth it.

So leave out this edge case.

[TristanH]

Gotcha -- that's what I suspected. Thanks for the detailed explanation, the symbol table would be awesome if it was implemented, but I agree probably out of scope :)

In that case I think this PR should be good to go!

[duaraghav8]

It is :) Allow me to review all other PRs, merging will begin in ~2 days

[duaraghav8]

@TristanH merged!
but There's a tiny issue - any special reason you used calleeNode.property.type in isSend()? The AST nodes don't have a property attribute (test fails)

Also callee.parent is undefined, so isSend function always gets sent an undefined and thus causes exception.

[duaraghav8]

20 passing (326ms)
  1 failing

  1) [RULE] no-send: Rejections
       should reject contracts using send:
     TypeError: Cannot read property 'type' of undefined
      at isSend (rules/no-send.js:10:20)
      at EventEmitter.inspectCallExpression (rules/no-send.js:34:8)
      at EventGenerator.leaveNode (node_modules/solium/lib/utils/node-event-generator.js:36:16)
      at Controller.leave (node_modules/solium/lib/solium.js:162:24)
      at Controller.exec (node_modules/sol-explore/lib/traverse.js:99:21)
      at Controller.traverse (node_modules/sol-explore/lib/traverse.js:148:8)
      at node_modules/sol-explore/lib/traverse.js:137:17
      at Array.forEach (native)
      at Controller.traverse (node_modules/sol-explore/lib/traverse.js:133:22)
      at node_modules/sol-explore/lib/traverse.js:140:18
      at Array.forEach (native)
      at node_modules/sol-explore/lib/traverse.js:139:11
      at Array.forEach (native)
      at Controller.traverse (node_modules/sol-explore/lib/traverse.js:133:22)
      at node_modules/sol-explore/lib/traverse.js:137:17
      at Array.forEach (native)
      at Controller.traverse (node_modules/sol-explore/lib/traverse.js:133:22)
      at node_modules/sol-explore/lib/traverse.js:140:18
      at Array.forEach (native)
      at node_modules/sol-explore/lib/traverse.js:139:11
      at Array.forEach (native)
      at Controller.traverse (node_modules/sol-explore/lib/traverse.js:133:22)
      at node_modules/sol-explore/lib/traverse.js:140:18
      at Array.forEach (native)
      at node_modules/sol-explore/lib/traverse.js:139:11
      at Array.forEach (native)
      at Controller.traverse (node_modules/sol-explore/lib/traverse.js:133:22)
      at Object.module.exports [as traverse] (node_modules/sol-explore/lib/traverse.js:169:28)
      at EventEmitter.lint (node_modules/solium/lib/solium.js:154:14)
      at Context.<anonymous> (test/no-send.js:28:20)

[TristanH]

Oh crap, sorry about that @duaraghav8 I'm looking now.

[TristanH]

Ah, I just passed in callee.parent instead of just callee to isSend on line 35... fix incoming. Sorry about that, no clue how it ended up there.

[TristanH]

And yeah, also property is not defined sometimes (I believe it is only defined when the function is an attribute of an object, otherwise it's undefined. Should have tested more!) Will address that too.