ERC827Token's method `transfer` not properly overridden

[carloschida]

🎉 Description

Assuming that the intention of the method transfer(address _to, uint256 _value, bytes _data) public returns (bool) of the ERC827Token interface is to handle ultimately the transfer of a token, and therefore its approval as well, I override it in my implementation but the behaviour does not math the expected result. My guess is that it has to do with the inheritance of the contract.

• [X ] 🐛 This is a bug report.
• [ ] 📈 This is a feature request.

Tutorial Token inheritance (simple).png:

See also ./UML/Tutorial Token inheritance.png of the repository carloschida/oz-workspace for the extended, annotated version with the method declarations in each contract.

💻 Environment

zeppelin-solidity: 1.6.0 Ganache 1.0.2 Truffle v4.0.6 (core: 4.0.6) Solidity v0.4.19 (solc-js)

📝 Details

Consider the final stage of the tutorial Robust Smart Contracts with OpenZeppelin that employs the tutorialtoken truffle box. So far everything works; truffle compiles it and migrates it.

Add properties to the contract such that tokens can be only transferred to pre-authorised addresses. For replicability I use the addresses generated by Ganache with the default mnemonic candy maple cake sugar pudding cream honey rich smooth crumble sweet treat.

Addresses of interest:

• A0 0x627306090abaB3A6e1400e9345bC60c78a8BEf57 (contract deployer)
• A1 0xf17f52151EbEF6C7334FAD080c5704D77216b732 (authorised address)
• A2 0xC5fdf4076b8F3A5357c5E395ab970B5B54098Fef (authorised address)
• A3 0x821aEa9a577a9b44299B9c15c88cf3087F3b5544 (non-authorised address)

Override the method transfer(address _to, uint256 _value, bytes _data) public returns (bool) with the corresponding logic, ie, implement somewhere in the method require(isAuthorized[_to]).

Note that line 63 ./src/js/app.js is the one that makes the call to the method return tutorialTokenInstance.transfer(toAddress, amount, {from: account});

Transfers:

• A0 -> A1 (works and it should work)
• A0 -> A3 (works but it should not work)

🔢 Code To Reproduce Issue [ Good To Have ]

This code is what should be added/modified to the latest state of the Robust Smart Contracts with OpenZeppelin. A repository containing the full code can be found here but I still leave the additions for them to be more easily readable.

TutorialToken.sol:

 pragma solidity ^0.4.4;

import 'zeppelin-solidity/contracts/token/ERC827/ERC827Token.sol';

contract TutorialToken is ERC827Token { // No longer (only) `StandardToken`
    string public name = 'TutorialToken';
    string public symbol = 'TT';
    uint8 public decimals = 0;
    uint public INITIAL_SUPPLY = 1000000;

    mapping(address => bool) public isAuthorized;

    address[2] authorizedAddresses = [
        // 0x627306090abaB3A6e1400e9345bC60c78a8BEf57, // A0 (contract deployer also non-authorised address)
        0xf17f52151EbEF6C7334FAD080c5704D77216b732,    // A1 (authorised address)
        0xC5fdf4076b8F3A5357c5E395ab970B5B54098Fef     // A2 (authorised address)
        // 0x821aEa9a577a9b44299B9c15c88cf3087F3b5544  // A3 (non-authorised address)
    ];

    function TutorialToken() public {
        totalSupply_ = INITIAL_SUPPLY;
        balances[msg.sender] = INITIAL_SUPPLY;
        // Authorise addresses A1 and A2
        isAuthorized[authorizedAddresses[0]] = true;
        isAuthorized[authorizedAddresses[1]] = true;
    }

    function transfer(address _to, uint256 _value, bytes _data) public returns (bool) {
        require(_to != address(this));
        require(isAuthorized[_to]); // <- CRITICAL
        super.transfer(_to, _value);
        require(_to.call(_data));
        return true;
    }
}

👍 Other Information

Note that if you additionally override the method function transfer(address _to, uint256 _value) public returns (bool) (attention to the signature) as follows, you obtain the desired result, ie, not being able to transfer to the non-authorised address A3.

function transfer(address _to, uint256 _value) public returns (bool) {
    require(_to != address(this));
    require(isAuthorized[_to]);
    super.transfer(_to, _value);
    return true;
}

This is why my guess is that something with inheritance is wrong.

[carloschida]

On the Slack channel, @vidorge commented that L63 of app.js actually invokes L31 of BasicToken.sol, ie function transfer(address _to, uint256 _value) public returns (bool).

My confusion came from the fact that call in L31 of app.js has 3 parameters but the last one is not meant to reach the contract.

He also said that one must always duplicate the logic for both methods and that my overriding of the three-parameter method should not be

function transfer(address _to, uint256 _value, bytes _data) public returns (bool) {
        require(_to != address(this));
        require(isAuthorized[_to]); // <- CRITICAL
        super.transfer(_to, _value);
        require(_to.call(_data));
        return true;
    }

but rather look like

function transfer(address _to, uint256 _value, bytes _data) public returns (bool) {
        require(_to != address(this));
        require(isAuthorized[_to]);
        super.transfer(_to, _value, _data); // Change here
        // require(_to.call(_data));
        return true;
    }

I'm trying it out now.

[carloschida]

My issue was solved, although I still need to understand web3's tricky way of forcing the inclusion of an extra parameter.

I updated both branches of my repository accordingly:

• master (here) stays as intended to reproduce the issue and/or future reference.
• resolution (here) was added to show the resolution of the problem in TutorialToken.sol.

Thank you, once again, @vidorge!

I leave the issue open though should anyone be so kind to comment a bit on:

• why both functions must contains the desired logic;
• why web3 forces a contract's methods to be called with that extra parameter;
• what _to.call(_data) does and whether it is needed in the overriding.