Openzeppelin-contracts: MintableToken#mint works for zero amounts

[maraoz]

See: https://github.com/OpenZeppelin/zeppelin-solidity/blob/8e01dd14f9211239213ae7bd4c6af92dd18d4ab7/contracts/token/MintableToken.sol#L34

IMO mint should revert if called with an amount of 0.

[spalladino]

@maraoz we discussed this last week, and we're not sure this is agood idea.

First, mint can be viewed as a transfer from 0x0 (as the standard suggests to signal via an event). Given that transferring zero amount is allowed, it would be consistent to also allow to mint zero.

Also, this catch makes sense if a human being is calling the mint function. However, for a smart contract which at some point decides to invoke mint on a token, this change forces it to check for the value to be not-zero before calling the function; which is not a standard requirement, and may cause the entire calling transaction to unexpectedly fail.

Given that minting zero does not pose a security risk, and to follow the robustness principle (_Be conservative in what you send, be liberal in what you accept_), I'd suggest not implementing this change.

[maraoz]

I don't see the advantage of allowing minting 0 tokens. It doesn't make sense in any situation, so I thought it was better for it to just fail. It's a good way to catch mint-function-user errors and prevent them from silently failing.

For example, adding this check would fix this toy problem: https://docs.google.com/presentation/d/1_SiLRCTpypJvsyjgOMWgOQahTrUDexuWJQQNHEvC2A0/view#slide=id.g2816afc9f9_0_441

This approach is in line with our principles D0 and D4. (https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/CONTRIBUTING.md#d0---security-in-depth)

edit: it would solve that problem because the final mint call would fail for a msg.value == 0.

[spalladino]

The advantage is allowing a smart contract that invokes mint on a token to be able to run a mint(0) call without needing to add extra checks. Even though mint is not part of the standard as transfer is, it still bears some resemblance, and since a contract should be able to safely call transfer(0) without needing to add any checks, the same should apply to mint.

That being said, feel free to sudo on this and I'll happily merge the associated PR ;-)

[maraoz]

My question was: when would a contract ever do mint(0)?

[spalladino]

Hmm let's say we have a token that mints a fraction of every transfer, and sends it to a _collector_ wallet.

contract TokenWithInflationPerTransfer is MintableToken {

  (...)

  address public collector;

  function transfer(address _to, uint256 _value) public returns (bool) {
    if (super.transfer(_to, _value)) {
      mint(_to, _value * 0.01);
      return true;
    }
  }
}

The code above works with the current version, but would break if we impose a validation for mint being greater than zero, forcing this contract to add a check for value * 0.01 > 0.

It's a contrived example, but I wanted to illustrate a scenario where zero is a legal value to process (such as in an ERC20 transfer), and is relayed to mint or other method.

[maraoz]

I'm convinced. I couldn't think of any example like the one above. Closing