argotorg / argotorg/solidity

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

Open
#11,886 11 comments 1 reaction 0 assignees View on GitHub
high effort high impact language design :rage4: must have
Dominant language
C++
Stars
25.7k
Forks
6.2k
Avg merge
2d 19h
Merged PRs (30d)
29

Description

# 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

```solidity
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

Contributor guide

Open the contributing guide

Research direction

Start by reproducing the supplied Solidity example and the described revert from return-value decoding. Then trace how the compiler handles try/catch boundaries, decoding, and statements inside the try block; the issue is done only when the intended behavior is defined and covered by regression tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, solidity
Domain
blockchain, compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.