BM-476: Callbacks

[willpote]

• Callbacks are only ever executed once, the first time a request is fulfilled
• Add a Callback interface and base contract for apps.
• In the non-locked path, callbacks are stored along with the price, when the request is "priced"

[willpote]

Thinking about this a bit more, the fact that the requestor can control the contents of the callback, specifically the journal, feels somewhat risky to me. I can't think of an immediate attack against the current construction, but it feels somewhat close in nature to a attack I have seen against a contract that made user-specified calls and the attacker used this to call transferFrom and steal funds that had been approved to the contract.

It's very possible this is fine, but I do have two ideas for mitigations.

• Pass the callbacks through a proxy that is intended to be untrusted (i.e. no one should ever provide is an ERC20 approval or grant it a privileged role in any way). The market contract itself will only ever call the proxy.
• Remove the journal from the directly sent data and instead stash it in transient storage. A call from the called contract can get journal's digest or contents. This has a few challenges though, including storing arbitrary length byte strings in transient storage.

Not sure what the right level of caution is here, or the right mitigation if caution is warranted.

Is this not a problem with the image that the callback implementer has chosen to use? If it is possible for the image to generate a journal that leads to tokens being transferred to the wrong person, then the image is the problem?

One thing this did make me think of is, would make sense to pass in the Predicate also used for the journal into the callback? Then even for more general purpose images, the callback can verify the journal meets some requirements? [I guess this would require us making the assessor output the predicate]

Re: the two mitigations, I don't fully see how they help. Even with a proxy, or fetching the journal from storage, ultimately there is a contract that needs to read the journal and decide what to do as a result of it- and if the journal can be manipulated, that contract will do the wrong thing.

[willpote]

Another thought, it probably makes sense for the callback template to have some functionality built into it around checking the image id is as expected, as that does feel important. Will add something

[nategraf]

Thinking about this a bit more, the fact that the requestor can control the contents of the callback, specifically the journal, feels somewhat risky to me. I can't think of an immediate attack against the current construction, but it feels somewhat close in nature to a attack I have seen against a contract that made user-specified calls and the attacker used this to call transferFrom and steal funds that had been approved to the contract.
It's very possible this is fine, but I do have two ideas for mitigations.

• Pass the callbacks through a proxy that is intended to be untrusted (i.e. no one should ever provide is an ERC20 approval or grant it a privileged role in any way). The market contract itself will only ever call the proxy.
• Remove the journal from the directly sent data and instead stash it in transient storage. A call from the called contract can get journal's digest or contents. This has a few challenges though, including storing arbitrary length byte strings in transient storage.

Not sure what the right level of caution is here, or the right mitigation if caution is warranted.

Is this not a problem with the image that the callback implementer has chosen to use? If it is possible for the image to generate a journal that leads to tokens being transferred to the wrong person, then the image is the problem?

One thing this did make me think of is, would make sense to pass in the Predicate also used for the journal into the callback? Then even for more general purpose images, the callback can verify the journal meets some requirements? [I guess this would require us making the assessor output the predicate]

Re: the two mitigations, I don't fully see how they help. Even with a proxy, or fetching the journal from storage, ultimately there is a contract that needs to read the journal and decide what to do as a result of it- and if the journal can be manipulated, that contract will do the wrong thing.

My description of the potential issue is not super clear, but the issue I was thinking of would require an coincidental collision in the value of the 4 byte selector, and sender-based authentication, as well as meeting other constraints on how it interprets the received data. It is unlikely that this will ever be the case.

[github-actions]

🚀 Documentation Preview

Deployment URL: https://boundless-documentation-4146xiet0-risczero.vercel.app

Updated at: 2025-03-05 15:02:08 UTC

[willpote]

I merged in and changed the base to be the unaggregated proof PR so that I could use the new types there

[nategraf]

Looks good to me!

I was staring at the logic in fulfillBatch again wondering if we really need 3 branches there. I think I have a nice simplification in that regard by moving the callback before fulfillment is set and using the fact that if anything fails, the whole call will revert anyway.

#364

[nategraf]

I think this is fine to include, and possibly beneficial, but its not clear to me that this is case. Let's keep this check for now, and clearly folks can always override this function to remove it if they decide it's unnecessary.

Keep in mind that anyone can specify a callback to any contract, and anyone can create a request on the market. So the amount of constraint this applies to the call is limited, beyond the fact that the receipt is already checked, which is rechecked based on our discussion earlier.