From 92eea9a6af0ddb313027ef455094e34138d74ff6 Mon Sep 17 00:00:00 2001 From: Roel Storms Date: Thu, 16 Jan 2025 13:25:02 +0100 Subject: [PATCH] Update inter-canister-calls.mdx to reference message execution properties (#3955) The properties used to be documented on this page. They moved to another page. The references were inaccurate. --- .../security-best-practices/inter-canister-calls.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/developer-docs/security/security-best-practices/inter-canister-calls.mdx b/docs/developer-docs/security/security-best-practices/inter-canister-calls.mdx index 5f4796e3b2..508617c0fd 100644 --- a/docs/developer-docs/security/security-best-practices/inter-canister-calls.mdx +++ b/docs/developer-docs/security/security-best-practices/inter-canister-calls.mdx @@ -20,7 +20,7 @@ This is also explained in the [community conversation on security best practices ### Security concern -Traps and panics roll back the canister state, as described in Property 5 above. So any state change followed by a trap or panic can be risky. This is an important concern when inter-canister calls are made. If a trap occurs after an await to an inter-canister call, then the state is reverted to the snapshot before the inter-canister call’s callback invocation, and not to the state before the entire call. +Traps and panics roll back the canister state, as described in [Property 5](/docs/current/references/message-execution-properties#message-execution-properties). So any state change followed by a trap or panic can be risky. This is an important concern when inter-canister calls are made. If a trap occurs after an await to an inter-canister call, then the state is reverted to the snapshot before the inter-canister call’s callback invocation, and not to the state before the entire call. More precisely, suppose some state changes are applied and then an inter-canister call is issued. Also, assume that these state changes leave the canister in an inconsistent state, and that state is only made consistent again in the callback. Now if there is a trap in the callback, this leaves the canister in an inconsistent state. @@ -135,11 +135,11 @@ GoldDAO's GLDT-swap has an implementation of journaling. In their case, the jour ### Security concern -As described in the [properties of message executions on ICP](/docs/current/references/message-execution-properties), messages (but not entire calls) are processed atomically. In particular, as described in Property 4 in that document, messages from interleaving calls do not have a reliable execution ordering. Thus, the state of the canister (and other canisters) may change between the time an inter-canister call is started and the time when it returns, which may lead to issues if not handled correctly. These issues are generally called 'Reentrancy bugs' (see the [Ethereum best practices on reentrancy](https://consensys.github.io/smart-contract-best-practices/attacks/reentrancy/)). Note however that the messaging guarantees, and thus the bugs, on ICP are different from Ethereum. +As described in the [properties of message executions on ICP](/docs/current/references/message-execution-properties), messages (but not entire calls) are processed atomically. In particular, as described in [Property 4](/docs/current/references/message-execution-properties#message-execution-properties) in that document, messages from interleaving calls do not have a reliable execution ordering. Thus, the state of the canister (and other canisters) may change between the time an inter-canister call is started and the time when it returns, which may lead to issues if not handled correctly. These issues are generally called 'Reentrancy bugs' (see the [Ethereum best practices on reentrancy](https://consensys.github.io/smart-contract-best-practices/attacks/reentrancy/)). Note however that the messaging guarantees, and thus the bugs, on ICP are different from Ethereum. Here are two concrete and somewhat similar types of bugs to illustrate potential reentrancy security issues: -- **Time-of-check time-of-use issues:** These occur when some condition on global state is checked before an inter-canister call, and then wrongly assuming the condition still holds when the call returns. For example, one might check if there is sufficient balance on some account, then issue an inter-canister call and finally make a transfer as part of the callback message. When the second inter-canister call starts, it is possible that the condition which was checked initially no longer holds, because other ledger transfers may have happened before the callback of the first call is executed (see also Property 4 above). +- **Time-of-check time-of-use issues:** These occur when some condition on global state is checked before an inter-canister call, and then wrongly assuming the condition still holds when the call returns. For example, one might check if there is sufficient balance on some account, then issue an inter-canister call and finally make a transfer as part of the callback message. When the second inter-canister call starts, it is possible that the condition which was checked initially no longer holds, because other ledger transfers may have happened before the callback of the first call is executed (see also [Property 4](/docs/current/references/message-execution-properties#message-execution-properties)). - **Double-spending issues.**: Such issues occur when a transfer is issued twice, often because of unfavorable message scheduling. For example, suppose you check if a caller is eligible for a refund and if so, transfer some refund amount to them. When the refund ledger call returns successfully, you set a flag in the canister storage indicating that the caller has been refunded. This is vulnerable to double-spending because the refund method can be called twice by the caller in parallel, in which case it is possible that the messages before issuing the transfer (including the eligibility check) are scheduled before both callbacks. A detailed explanation of this issue can be found in the [community conversation on security best practices](https://www.youtube.com/watch?v=PneRzDmf_Xw&list=PLuhDt1vhGcrez-f3I0_hvbwGZHZzkZ7Ng&index=2&t=4s). @@ -309,7 +309,7 @@ Finally, note that the same guard can be used in several methods to restrict par ### Security concern -As stated by the Property 6 above, inter-canister calls can fail in which case they result in a **reject**. See [reject codes](/docs/current/references/ic-interface-spec#reject-codes) for more detail. The caller must correctly deal with the reject cases, as they can happen in normal operation, because of insufficient cycles on the sender or receiver side, or because some data structures like message queues are full. +As stated by the [Property 6](/docs/current/references/message-execution-properties#message-execution-properties), inter-canister calls can fail in which case they result in a **reject**. See [reject codes](/docs/current/references/ic-interface-spec#reject-codes) for more detail. The caller must correctly deal with the reject cases, as they can happen in normal operation, because of insufficient cycles on the sender or receiver side, or because some data structures like message queues are full. Not handling the error cases correctly is risky: For example, if a ledger transfer results in an error, the callback dealing with that error must interpret it correctly. That is, it must be interpreted as "the transfer did not happen".