Clarification needed for SWC-123

[muellerberndt]

The description & sample for SWC-123 - Requirement Violation doesn't make sense to me.

Generally, require() is used for input validation and it the condition being violated is expected behavior.

The description seems to refer to a special case where "a contract provides the external input". I assume that the point of this is that if somebody creates a system that contains multiple contracts, the system should be consistent such that no contract calls another contract with invalid inputs?

This should be reflected in the SWC title and could also use a clearer description.

[wuestholz]

I'm not quite sure I fully understand the question.

The case where the external input is provided by a contract is the only case we care about here. If users provide invalid inputs it's clearly their fault. Is this what you would like to be clarified (e.g., by changing the title to "requirement violation by contract")?

[muellerberndt]

Yep, to me the title "Requirement Violation" would indicate that requirement violations are always "wrong". However, the way I understand it, require() is the recommended way to catch invalid inputs, so as a tool author I would not report this as a something relevant.

IMO if the require() validates return values from external callees, it makes no difference if the callee is a user or a contract. It is still input validation.

The distinction here is whose fault it is that an invalid input was provided?

In most cases, such external inputs are provided by callers, but they may also be returned by callees. In the former case, we refer to them as precondition violations.

As what do we refer to them in the latter case?

[muellerberndt]

For clarification:

pragma solidity ^0.4.25;

contract Bar {
    Foo private f = new Foo();
    function doubleBaz() public view returns (int256) {
        return 2 * f.baz(0);
    }
}

contract Foo {
    function baz(int256 x) public pure returns (int256) {
        require(0 < x);
        return 42;
    }
}

In this case, we report a weakness in Bar because it calls f.baz() in a way that always fails?

Would SWC-123 also apply if doubleBaz was changed to:

    function doubleBaz(int256 user_input) public view returns (int256) {
        return 2 * f.baz(user_input);
    }

[wuestholz]

I would say requirement violations are always "wrong". However, it's more challenging to determine who is to blame (see the two cases in the description about how to fix them). Usually, the external entity (contract or user) is to blame (e.g., precondition violation), but it might also be that the requirement is overly strong.

I don't think there is a standard name for referring to cases where the callee returns invalid outputs (maybe postcondition violation although here the check happens in the caller which is a bit unusual). But I don't think the naming is that important here since I suspect this case is much less common.

About your example: Yes, in your modified example I would argue that doubleBaz is to blame since it may provide invalid inputs to baz.

[muellerberndt]

Hmm, yeah in a sense requirement violations are always "wrong" when happening during runtime, but the use of a require statement by the programmer for catching a possible violation isn't wrong.

Compare this with SWC-110 - Assert Violation. An assert violation necessarily shows that either there's something wrong with the contract or assert is being used wrongly.

I think using the title from CWE would make this clearer. Maybe split this up into 2 SWCs:

• Improper Following of Specification by Caller: A weakness exists in x if x calls a function in y in a way that could violate a requirement in y.

• Improper Following of Specification by Callee: Aa weakness exists in y if a function in y, when called by x, could return a value that violates a requirement in x.

[wuestholz]

Yes, using require statements is perfectly fine to filter invalid inputs. Essentially, by adding them, the programmer implicitly introduces assertions that external contracts or users should adhere to.

I can see why one might want to split the two cases up. However, I feel like it might complicate things for tools without making the description much simpler. In particular, I'm afraid that tools won't be able to automatically decide which weakness ID (between the two cases) to assign. Obviously, this is easy for some cases (e.g., without calls before the violation), but there can be cases where (caller) arguments are validated even after calls to other contracts and then things can get much more complicated. What do you think?

[maurelian]

So, what's the conclusion here?
It seems like it's a tradeoff between a desire for a well defined and automatable description, vs. a more ambiguous description that maps more closely to the fuzziness of reality?