CORIONplatform / solidity

GNU General Public License v3.0
12 stars 9 forks source link

Mist Contract Wallet usage inconsistency #119

Closed gundas closed 7 years ago

gundas commented 7 years ago

I think it is kind of a usability problem - people will not understand why they CORION transfers are failing.

If I use Mist Multisig Wallet Contract (https://github.com/ethereum/mist/wiki/Wallet-Contract), I can buy tokens using ICO contract.

But, because of the way token contract's transfer and transferFrom functions are implementated https://github.com/CORIONplatform/solidity/blob/master/token.sol#L196-L197 my Wallet Contract will not be able to receive any CORION transfers.

Not sure about the fix - maybe the code should not fail if the contract does not implement the receiveCorionToken method (but it should still fail the contract implements the function but the call fails).

P.s. To add more confusion to the matter, the transferFromByModule does not implement the same logic regarding contracts as the transfer and transferFrom functions. - I understand, this transfer is currently used only to transfer CORIONs between modules.

gundas commented 7 years ago

Hmm, this needs more testing on testnet, but I still think it is broken.

In Remix, the receiveCorionToken call to Wallet contract succeeds because the Wallet contract defines a fallback function. However, the return values are messed up because the fallback function does not return anything - e.g. I get back success = true and _back = 269599466671506397946670150870196306736371444225405724811036102492160. The "return" values are dependent on the parameters.

iFA88 commented 7 years ago

Sadly we dont have any accepted token standard (ERC223 is still draft). If you call from contract an transfer for other contract, both contract need implement CORION dependencies.

Dexaran commented 7 years ago

ERC223 is still draft

This is not a valuable argument. ERC20 is a draft too but nearly all Ethereum tokens are ERC20 standard tokens. All token standards are drafts.

gundas commented 7 years ago

I still find it very inconsistent - a contract which does not implement receiveCorionToken:

On the contrary, a contract which does implement receiveCorionToken does not get informed if some funds are transferred back https://github.com/CORIONplatform/solidity/blob/master/token.sol#L310

gundas commented 7 years ago

Also, please note that a result of a call to a contract which does not implement receiveCorionToken but has a fallback function is not predictable since it does not fail but returns some arbitrary data back - at least in Remix/Solidity Browser.

Dexaran commented 7 years ago

It seems strange. Fallback function doesn't return anything.

iFA88 commented 7 years ago

Mist wallet don't works with external calls return variables (Except if is it a constant). This means if you send any tokens from the Ethereum network, the Mist will not informed how many tokens are send. The mist check token.balanceOf for every block. This means, if I have 100 token and transfer 50 token for an contract which send backs 25 token, the Mist will display 75 token for my balance and this is correct. In the event log you will see, that A transfered to B 50 token and B transfered to A 25 token.

gundas commented 7 years ago

@Dexaran ,

I've just tested on rinkeby - created a simple Wallet Contract in Mist and deployed the following contract which calls my Wallet Contract:

contract shelling { 
// this contract is needed only for the other contract to compile - it's never actually called

    function receiveCorionToken(address, uint256, bytes) returns (bool, uint256){ 
        return (false, 222);
        }
} 

contract testcast {
    event RetVal(uint256 indexed value, bool indexed resultSuccess, uint256 indexed returnVal);

    // I call this method from Mist and provide my Wallet Contract address and some uint value
    function test(address addr, uint256 n) returns (uint256) {

        bytes memory _data;
        var (_success, _back) = shelling(addr).receiveCorionToken(addr, n, _data);
        RetVal(n, _success, _back);
        require(_success);
        return _back;
    }

What I get back in Mist within events RetVal is: value: 0 resultSuccess: true returnVal:16162315240658618788452431523768575880032480740854469139892582951366904774656

The returnVal changes depending on the input values but I do not understand the algorithm yet.

Dexaran commented 7 years ago

@gundas it seems to be EVM-related error. The return value is a value from EVM stack but I can't recognize what is causing it.

Dexaran commented 7 years ago

@gundas what compiler version do you use? I tested it all on Rinkeby and I didn't experienced any problems. Everything is working fine.

Dexaran commented 7 years ago

Confirmed. This is resulting in wrong return values when a contract with a fallback function is called. This is not a mist bug but it is a real bug.

iFA88 commented 7 years ago

The Token contract will check the third party contract, about its for CORION developed or not. If the variable is the same as the Token contract address, then okay. https://github.com/CORIONplatform/solidity/blob/rc/token.sol#L391-L393

We can not take any responsibility for the third party contract if that is wrong coded.