ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.29k stars 5.77k forks source link

`try`/`catch` for `abi.decode()` or `abi.tryDecode` #10381

Open gitpusha opened 3 years ago

gitpusha commented 3 years ago

Hi,

I am referencing the conversation #10317 .

It would be nice to have a way to catch errors thrown during abi.decode operations.

Quoting @chriseth:

Instead of try/catch we should maybe rather have abi.tryDecode? The problem here is how to return the data in the two cases

Also try catch might be useful for functions other than abi.decode but I will first need to find concrete examples and will report them here in this issue.


Preliminary implementation: https://github.com/ethereum/solidity/pull/10883

chriseth commented 3 years ago

I'm starting work on the underlying mechanism for use in catch. The problem of exposing this to the user is still how to deal with the actual values in case of an error. We can use a success flag:

(bool success, uint value) = abi.tryDecode(ms.data, (uint));

but this would make value accessible even in case of failure.

ekpyron commented 3 years ago

I'm starting work on the underlying mechanism for use in catch. The problem of exposing this to the user is still how to deal with the actual values in case of an error. We can use a success flag:

(bool success, uint value) = abi.tryDecode(ms.data, (uint));

but this would make value accessible even in case of failure.

Algebraic datatypes (https://github.com/ethereum/solidity/issues/909) could be a way around that...

chriseth commented 3 years ago

Until we have algebraic datatypes, returning an additional bool might be a good solution. All our types should have a sane zero-value, at least those that can be abi-decoded.

daltonclaybrook commented 2 years ago

Any update on this issue? Would love to be able to try abi.decode without fear of panic.

fredlacs commented 2 years ago

Would also love support for abi.tryDecode that returns a bool

giuseppeg commented 2 years ago

What is the current solution to this? Just let it revert without reason?

CJ42 commented 2 years ago

It would be useful especially in case when trying to decode abi-encoded arrays, where the bytes data passed as an argument to abi.decode(...) is not a properly encoded array of valueTypes[], according to the ABI specs.

I encountered the issue on my side, and the only way was to write some custom functions that analyse the data to check for valid offsets, length, etc...

For reference, see https://github.com/lukso-network/lsp-smart-contracts/blob/53e33cc5359244cbc6f9f1db4ffc7c5b1348ad67/contracts/LSP2ERC725YJSONSchema/LSP2Utils.sol#L135-L197

using BytesLib for uint256;

function isEncodedArray(bytes memory data) internal pure returns (bool) {
    uint256 nbOfBytes = data.length;

    // there must be at least 32 x length bytes after offset
    uint256 offset = uint256(bytes32(data));
    if (nbOfBytes < offset + 32) return false;
    uint256 arrayLength = data.toUint256(offset);

    //   32 bytes word (= offset)
    // + 32 bytes word (= array length)
    // + remaining bytes that make each element of the array
    if (nbOfBytes < (offset + 32 + (arrayLength * 32))) return false;

    return true;
}

function isEncodedArrayOfAddresses(bytes memory data) internal pure returns (bool) {
    if (!isEncodedArray(data)) return false;

    uint256 offset = uint256(bytes32(data));
    uint256 arrayLength = data.toUint256(offset);

    uint256 pointer = offset + 32;

    for (uint256 ii = 0; ii < arrayLength; ii++) {
        bytes32 key = data.toBytes32(pointer);

        // check that the leading bytes are zero bytes "00"
        // NB: address type is padded on the left (unlike bytes20 type that is padded on the right)
        if (bytes12(key) != bytes12(0)) return false;

        // increment the pointer
        pointer += 32;
    }

     return true;
}

function isBytes4EncodedArray(bytes memory data) internal pure returns (bool) {
    if (!isEncodedArray(data)) return false;

    uint256 offset = uint256(bytes32(data));
    uint256 arrayLength = data.toUint256(offset);
    uint256 pointer = offset + 32;

    for (uint256 ii = 0; ii < arrayLength; ii++) {
        bytes32 key = data.toBytes32(pointer);

        // check that the trailing bytes are zero bytes "00"
        if (uint224(uint256(key)) != 0) return false;

        // increment the pointer
        pointer += 32;
    }

    return true;
}

a tryDecode would be useful in such case and apply well. For instance:

function isAbiEncodedArrayOfAddresses(bytes memory data) public returns (bool) {
    try abi.tryDecode(data, (address[]) returns (address[] memory) {
        // ...
        return true;
    } catch {
        // ...
        return false;
    }
}

Or in a more simpler using @chriseth example:

function isAbiEncodedArrayOfAddresses(bytes memory data) public returns (bool) {
    (bool success,) = abi.tryDecode(ms.data, (address[]));
    return success;
}

For arrays, the tryDecode could analyze the structure of the bytes given as input. For instance for abi-encoded array of bytes:

For simple types like uint256, uint128, bytesN, etc..., I guess it could check for things like out of range value, the right number of bytes, etc...

novaknole commented 2 years ago

Hi all.

Any news on this ?

cameel commented 2 years ago

Not yet, other that we do consider this pretty important so it's on our roadmap, along with other high-impact features. It's waiting for its turn.

jefflau commented 1 year ago

Hi all. I'm also looking for this feature, would be nice to be able to catch and throw a specific error

chriseth commented 1 year ago

Preliminary implementation: https://github.com/ethereum/solidity/pull/10883

cameel commented 1 year ago

We have too many duplicates of this issue. I'm going to close it in favor of #13869, which has a syntax proposal that could use some feedback.

cameel commented 1 year ago

Ah, wait. This is actually slightly different. #13869 would still not allow you to catch reverts from explicit abi.decode() calls. Just from the implicit decoding that the compiler does. Still, maybe allowing abi.decode() in try/catch and using the syntax from https://github.com/ethereum/solidity/issues/13869#issuecomment-1422879881 would be a good way to solve this.