`try`/`catch` doesn't catch reverts during decoding or inside the `try` block

[drortirosh]

The problem:

try/catch construct was added to allow easily handle reverts when calling functions. However, it gives false sense of security, since there are cases where reverts are "leaked" through

Example

The following code looks safe, since the call is covered by catch-all try/catch. But in fact, it reverts

interface Istr {
    function retstr() external view returns(string memory);
}

contract Test {
    
    function asd() external {
        try Istr(address(this)).retstr() returns(string memory ret) {
            emit Debug(ret);
        } catch {
            emit Debug('reverted');
            
        }
    }

    function retstr() external  returns (uint) {
        return 1;
    }
    event Debug(string mesg);
}

Root cause

while try/catch does catch all reverts by the external call, it doesn't handle any local handling of the response. The above example receives a "uint" return value, but tries to parse it as dynamic content (string), and the decoder reverts.

There are other cases where the user might use checked arithmetics between the try/catch, which assumes it get caught by "catch"

Suggested fix

• At a minimum, the return value decoding should not revert in a try/catch block. otherwise, try/catch can't be trusted when calling unknown contracts, and developers must resort to low-level address.call(abi.encodeWithSelector(...)) (and manually validate the returned structure before decoding)

In addition, the try/catch model should be updated:

• in case a user specifies a catch {} block, it is his expectation that no revert will leak from the "try" block.
• The entire code after the "try" until the "catch" should be "no-revert" - must not generate revert by the compiler itself.
• Instead of reverting, this code should jump to the catch {} block
• Implementing a full "stack unwinding" for revert handling seems unlikely for solidity, so instead I suggest:
  • when compiling methods, mark each method as "may-revert" or "no-revert"
  • Is case the try/catch block calls "may-revert" method, a compilation error should be generated: cannot call method X from a try/catch block: the method might revert
  • the user should then alter his code and call that method outside the try/catch block, or add "unchecked" markers to that method

[cameel]

Related issue: #9592 (try/catch with low-level calls).

[chriseth]

The idea behind the current behaviour was that if you end up in the catch-part of the try/catch block, then you know that the call has reverted. I think this is a very important property that we should keep for safety and it is the better tradeoff than the suggested change.

[chriseth]

We could add another clause that is only executed on call-related errors (either no code at destination or decoding error):

try ... {
} catch ... {
} callError {
} decodeError {
}

[cameel]

I think we might be better off closing this and reformulating the problem to be more focused. As is it is now, it touches upon multiple things:

• Local reverts coming from decoding and other extra checks on the external call cannot be handled. This is now covered by #13869 (and we have a syntax proposal there).
• Reverts in the try block are not being caught. I agree that it's counter-intuitive and maybe worth adjusting the syntax so that users do not expect that much from it. It would be breaking though so definitely not before 0.9.0. One idea to deal with this might be to eliminate that block and replace it with an else {} block like in Python.
• Actually catching reverts from the try block with either full unwinding or a "may-revert" hack. Unfortunately I don't think we'd be implementing this in the near future. I won't say never, but in the short term we'd like to patch the try/catch to be good enough in the current form and focus on other things on the roadmap. In the meantime we can of course discuss ideas for the future of try/catch but that would be better done on the forum.

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

[drortirosh]

Please remove the "stale" tag, as I think it is a still a serious problem to fix. It which will catch (pun intended) unsuspecting developers that are used to "catch" behavior in other languages.

[CJ42]

The problem of try / catch in Solidity is also that if the external call is made to a function in a contract, but the data returned by the call is not of the same expected type (e.g: you expect a bytes4, but the function returns a bytes32), the return part of the try will revert but not fall into any catch clause (because of ABI decode failing issue I suppose?)

A good extra clause + type of error would be:

} catch Error(uint256 /* errorCode */) {
   // catch revert errors related to ABI encoding / decoding, etc...
}

See this forum thread: https://forum.soliditylang.org/t/call-for-feedback-the-future-of-try-catch-in-solidity/1497?source=post_page-----c59507d7d7d5--------------------------------

[novaknole]

Hi all.

Is there a plan to make try/catch more error-prone or it will stay as it is(basically, all the comments in this thread) ?

[cameel]

For the time being it's on hold. If there was a simple and straightforward way to fix its quirks in a non-breaking way, we might do it, but any deeper changes are not worth it at this point. We prefer to invest that time into finishing the new type system and when we have that we'll likely just retire try/catch and replace it with a simpler and more consistent mechanism.

[novaknole]

Thanks @cameel for the follow up. ^_^