CORIONplatform / solidity

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

Unexpected return values. #123

Closed Dexaran closed 7 years ago

Dexaran commented 7 years ago

When a contract with empty fallback function is called receiveCorionToken function is returning unexpected values.

Dexaran commented 7 years ago

pragma solidity ^0.4.11;

contract test_receive
{ 
// this contract is needed only for the other contract to compile - it's never actually called
    function receive(address, uint256, bytes) returns (bool, uint256)
   { 
        return (false, 222);
    }
} 

contract test_cast
{
    uint public test_uint;
    bool public test_bool;

    function test(address addr, uint256 n) returns (uint256)
    {
        bytes memory _data;
        var (_success, _back) = test_receive(addr).receive(addr, n, _data);
        require(_success);
        test_uint = _back;
        test_bool = _success;
        return _back;
    }
}

contract dummy
  {
    function() payable {  }
}

I've compiled contracts and deployed test_cast at this address on Rinkeby: 0x330D0f8cc94758F26DD299bb803Fe63Fef76F726.

I've deployed dummy here on Rinkeby: 0x66855c513B861b617aD806C9d4CC35661087dda3

I've called test at test_cast with this parameters: 0x330d0f8cc94758f26dd299bb803fe63fef76f726 , 124 .

Call tx: https://rinkeby.etherscan.io/tx/0x88833366a0ca22e1aacdb64c7d2e3aefa54e7f719782e36a5fbde1a078c482cd

In the result of the call test_uint became 7477059611491291558618241337412310647290875865618134341354521175411004538880 and test_bool became true .

I think that if the fallback function doesn't return any values then it should not return values depending on my input.

iFA88 commented 7 years ago

What is your suggestion?

Dexaran commented 7 years ago

I've reported the issue to Ethereum/Solidity.

I suppese that the only way to avoid possible mistakes right now is to avoid using return values in receiveCorionToken function execution.

iFA88 commented 7 years ago

I think you have explain above, that the contract what calling don't have the specified function, then calls the fallback which has no return variable, right? If this is right, that means the contract should not call some contract who are not implanted the right function. Maybe we should standardize an variable. Example for contract who uses CORION token:

address public corAddress = 0xabfc.....;

Before the receiveCorionToken the sender should check the thirdpartycontract(addr).corAddress == address(self) . If this are okay, then can send token to this contract.

What is your opinion?

Dexaran commented 7 years ago

It can solve a problem of sending COR tokens to the contract that is not designed to receive them.

It will not solve a problem if the malicious actor creates a contract with the implementation of this function:

address public corAddress = 0xabfc.....;

function() payable { }

and executes a transfer to this contract. He can still mess with returned values.

iFA88 commented 7 years ago

That's right, but you can't prepare for an unknown contract.. How do you check, that the third party contract is everything implemented what you need?

iFA88 commented 7 years ago

Its like you send ether to an contract who can not forward it and stuck there..

Dexaran commented 7 years ago

Yes, but you don't expect any return values when sending Ether. In case of COR token you use returns. ERC223 tokenFallback function is not returning something like a fallback function to receive Ether.

iFA88 commented 7 years ago

How can you check that what you send have successfully arrived and the contract can handle it (example forwarding tokens or ether). I think the address public corAddress = 0xabfc.....; solution is not bad. While you coding use this, you should know what is the requirements.

Dexaran commented 7 years ago

Return values should be avoided before Metropolis. I can suggest only to exclude return values because it is unsafe to use them before this will be fixed: https://github.com/ethereum/solidity/issues/2630#issuecomment-318091534

iFA88 commented 7 years ago

https://github.com/CORIONplatform/solidity/pull/137