FuelLabs/fuels-rs

Erroneous result value on reentrant calls

Closed this issue · 4 comments

ReceiptParser makes the wrong assumption that there is only a single Return receipt per contract id. This is not the case for reentrant calls.

For instance, executing the following codebase tests (with forc build && cargo test: https://github.com/IGI-111/reentrancy-repro

produces the following receipts:

[tests/harness.rs:43:5] res.receipts = [
    Call {
        id: 0x0000000000000000000000000000000000000000000000000000000000000000,
        to: 0xb7cac0a0521248f8baac940a0902bfd7fdc5b6bc1936b59a801df3b4a75483c8,
        amount: 0,
        asset_id: 0x0000000000000000000000000000000000000000000000000000000000000000,
        gas: 456,
        param1: 1619450810,
        param2: 10480,
        pc: 11960,
        is: 11960,
    },
    Call {
        id: 0xb7cac0a0521248f8baac940a0902bfd7fdc5b6bc1936b59a801df3b4a75483c8,
        to: 0x07822be5596a1039c73a41ffc1b8aa0b3cc0c9ffb98ebc563961b730f2e79795,
        amount: 0,
        asset_id: 0x0000000000000000000000000000000000000000000000000000000000000000,
        gas: 238,
        param1: 3752339489,
        param2: 10480,
        pc: 13168,
        is: 13168,
    },
    Call {
        id: 0x07822be5596a1039c73a41ffc1b8aa0b3cc0c9ffb98ebc563961b730f2e79795,
        to: 0xb7cac0a0521248f8baac940a0902bfd7fdc5b6bc1936b59a801df3b4a75483c8,
        amount: 0,
        asset_id: 0x0000000000000000000000000000000000000000000000000000000000000000,
        gas: 64,
        param1: 1003047684,
        param2: 0,
        pc: 14320,
        is: 14320,
    },
    Return {
        id: 0xb7cac0a0521248f8baac940a0902bfd7fdc5b6bc1936b59a801df3b4a75483c8,
        val: 1337,
        pc: 14388,
        is: 14320,
    },
    Return {
        id: 0x07822be5596a1039c73a41ffc1b8aa0b3cc0c9ffb98ebc563961b730f2e79795,
        val: 1337,
        pc: 13304,
        is: 13168,
    },
    Return {
        id: 0xb7cac0a0521248f8baac940a0902bfd7fdc5b6bc1936b59a801df3b4a75483c8,
        val: 42,
        pc: 12128,
        is: 11960,
    },
    Return {
        id: 0x0000000000000000000000000000000000000000000000000000000000000000,
        val: 1,
        pc: 10388,
        is: 10368,
    },
    ScriptResult {
        result: Success,
        gas_used: 646,
    },
]

The return value of this call with these receipts should be 42, as this is the original call done at is: 11960, however since there are two return receipts for contract 0xb7cac0a0521248f8baac940a0902bfd7fdc5b6bc1936b59a801df3b4a75483c8, ReceiptParser picks the wrong one and the return value is erroneously considered to be 1337.

The simple solution is to just use the last receipt that matches the contract id.

But this causes problems for multi-calls. If we were to call this contract multiple times in one script we can no longer use the last receipt since we don't know how many reentrancies there were.

E.g. in the above example if we were to do

        multi_call_handler
            .add_call(call_handler_1)
            .add_call(call_handler_2);

where call_handler_1 and call_handler_2 call methods on the same contract.

If both methods experience reentrancy then we have an unknowable (ahead of time) amount of receipts and can no longer do multicalls. The number would depend on how many (if any) times the contract was entered during the call to each method.

@IGI-111 is there anything that comes to mind that might alleviate the above?

Also don't have anymore time over the weekend. Gonna unassign myself so that somebody else might pick this up. Otherwise gonna continue on the offsite @digorithm .

Pushed the simple fix for Data and ReturnData here for anyone continuing.

Heap types also need handling.

I think relying on the order alone is going to be error prone. Most likely you need to take the rest of the receipt information into account. (is in particular) But I haven't seen how this behaves in multi calls.

It may be that the VM needs to give more info to disambiguate or that the SDK needs to maintain some information about what gets called in what order.

Simple fix is not going to cut it for something as potentially exploitable as this.

I also have not checked if fuels-ts is affected, which it may be. I'll leave that to you guys to check.