Solidity: Missing return statement on functions do not issue an error

[mattaereal]

_This issue is being reported as part of the current audit being held by Zeppelin Solutions to Solidity's compiler (tag v0.4.24)._

Description

When a function is declared with a return parameter, but the function body has no return statement, the compiler does not show an error nor warns the user about this.

Environment

• Compiler version: v0.4.24

Steps to Reproduce

pragma solidity ^0.4.24;
contract Empty {
   int variableName;
   constructor() public {
       variableName = 0;
   }
   function emptyReturn() public pure returns (int) { }
   function setWithEmptyReturn() public {
       variableName = emptyReturn();
   }
}

Related issues

I tried to find if this was a duplicate, but couldn't find anyone reporting exactly the same.
I also saw you were working on both #4541 #2951 , and thought it would save you time if I report this one now, so you could decide where and how to respond to it before finishing other related issues.

[chriseth]

Note that this behaviour is on purpose. There is a warning as soon as the return parameter has a name. I still put it up for discussion. There might be a duplicate, though.

[ekpyron]

We could use the control flow analysis to warn (or even raise an error, although that may be too much), if there is any code path that leaves any return value unassigned - that would work for both named and unnamed return values. I think it's a good idea.

We should think about patterns like in https://github.com/ethereum/solidity/blob/42447a14ebc146dd3ff3034360e85bebe3f34ce6/test/compilationTests/milestonetracker/RLP.sol#L172, though, but I think this code is bad/dangerous anyways and should probably be changed.

[Mansoor-AR]

Note that this behaviour is on purpose

I don't see why this is the case. As I see it, if there is an unnamed return in the function declaration then the compiler must check for explicit returns and raise an error if one isn't found. For example, this is how Go behaves.

[robinbryce]

The fact that the following code compiles without error hid a stupid one line bug in our contract upgrade code. This cost us real money. What is the benefit of permiting explicitReturn in the following example to compile ?

If the answer is 'use named returns everywhere` (please no) why permit explicit returns at all ?

pragma solidity >=0.6.3;

contract ReturnTest {

    function explicitReturn() internal pure returns (uint[] memory) {
        uint[] memory x = new uint[](5);
        for (uint i=0; i<x.length; i++){
            x[i]=i;
        }
    }
    function namedReturn() internal pure returns (uint[] memory x) {
        x = new uint[](5);
        for (uint i=0; i<x.length; i++){
            x[i]=i;
        }
    }
}

[chriseth]

I don't see why you would declare another local variable in explicitReturn, but what about the following rule:

• if there is an unnamed return variable and
• the function body is not empty and
• there is no return statement

issue a warning.

[robinbryce]

That rule would have caught our bug. The bug being a function which generated some new data, by processing some input, and then failed to return anything at all. I'm not necessarily expecting deep flow analysis here. The static check you mention would be great.