mark-callable-contracts notified on superclass method calls

[mdnorman]

Superclasses cannot be renamed to follow the Trusted/Untrusted pattern, and they should always be trusted since they are imported and the source is known beforehand.

Example:

import '@openzeppelin/contracts-ethereum-package/contracts/token/ERC20/ERC20.sol';
import '@openzeppelin/contracts-ethereum-package/contracts/token/ERC20/ERC20Detailed.sol';
import '@openzeppelin/contracts-ethereum-package/contracts/token/ERC20/ERC20Mintable.sol';

contract GLDToken is ERC20, ERC20Detailed, ERC20Mintable {
  function initialize(uint256 initialSupply) public {
    ERC20Mintable.initialize(msg.sender);
    ERC20Detailed.initialize('Gold', 'GLD', 18);

    _mint(msg.sender, initialSupply);
  }
}

solhint output:

# npx solhint contracts/**/*.sol

contracts/GLDToken.sol
  10:5  error  Explicitly mark all external contracts as trusted or untrusted  mark-callable-contracts
  11:5  error  Explicitly mark all external contracts as trusted or untrusted  mark-callable-contracts

✖ 2 problems (2 errors, 0 warnings)

Expected behavior is to ignore the calls to ERC20Mintable.initialize(...) and ERC20Detailed.initialize(...)

[fvictorio]

Hi @mdnorman, thanks for reporting this.

I guess the exception should be for any imported contract, not only superclasses, right? For example, I don't know if library calls trigger a false positive for this error, but it shouldn't.

The problem in that case is that you have no way to know the libraries/contracts imported from a file, since Solhint only works at file-level (i.e., it doesn't look at other files while processing one).

I guess just handling superclasses might help... maybe? Although that seems like a half-measure. Alternatively, maybe this rule should accept some configuration of trusted contracts. Something like:

"mark-callable-contracts": ["warn", {
  "whitelist": ["ERC20", "ERC20Detailed", ...]
}]

That might get cumbersome but is the most flexible approach.

What do you think about this last option? Is it asking too much from the user?

[mdnorman]

Even if solhint only works at file-level, it can see the imports, and if it looks at the structure of the contract, it can parse out whether it's a variable or the name of an imported class.

I also ran into a problem with constructing structs, eg MyStruct({someField: someValue})

[mdnorman]

To answer your question about the rule config, I probably wouldn't whitelist the classes. Instead, I would just do what I'm doing now, which is to use // solhint-disable-next-line mark-callable-contracts

[fvictorio]

Even if solhint only works at file-level, it can see the imports, and if it looks at the structure of the contract, it can parse out whether it's a variable or the name of an imported class.

Yes, good point, I guess something can be done there. The imports don't actually matter because you don't have any guarantee that the imported name matches the file name, if that's what you're saying. But you could see if the variable is declared somewhere or if it's a parameter.

I think there would be some false negatives though. For example, if your contract inherits from another one, defined in a separate file, that has a state variable with a contract instance. In that case you would want to check it, but it wouldn't be triggered. This rule is full of edge cases.

I also ran into a problem with constructing structs, eg MyStruct({someField: someValue})

That sounds like a bug. Could you please open a new issue for that with a full example?

[mdnorman]

I see what you mean regarding the filename not matching the contract name.

I created the struct bug here: https://github.com/protofire/solhint/issues/198

Is the rule currently just looking for function calls on PascalCase? I noticed that if I have a variable named with lowerCamel, it doesn't seem to catch any problems.