Extension execution is not guaranteed

[nns2009]

Wallet v5 doesn’t guarantee that Extension request will be executed. The problem lies in these lines:

;; Note that some random contract may have deposited funds with this prefix,
;; so we accept the funds silently instead of throwing an error (wallet v4 does the same).
(_, int extension_found) = extensions.udict_get?(size::address_hash_size, sender_address_hash);
ifnot (extension_found) {
    return ();
}

It looks like authors have thought of this case, but didn’t realize the full consequences. The most basic way in which it becomes problematic:

• Extension attaches significant amount of TONs
• Extension expects the Wallet to send the attached message (or execute some other action)
• Wallet’s Owner quickly disables the Extension
• extension_found is false, and Wallet keeps all the attached TONs

(appropriate untranslatable Russian expression: зажимает деньги)

Example use case

Wallet Owner wins (or buys, or gets by personal connections, etc.) the right to participate in the ICO (or something else of the same kind). ICO has a list of white-listed addresses from which buy-requests are accepted, so most people can’t participate. Wallet Owner would like to sell the right to use his Wallet as a “Proxy” in exchange for some commission. Note that it is safe for Wallet Owner to allow such usage, because (s)he might take a look at the Extensions’ code and verify that the Extension attaches at least the amount it requests the Wallet (=Proxy) to send/forward further + some commission for the privilege/service. Unsuspecting Extension Owner requests to buy 1 000 000 TON worth of coins (or do some other transaction) through Wallet as a Proxy. Suspecting malicious Wallet Owner disables the extension at the right moment. 1 000 000 TON arrive to the Wallet, but the Extension had been disabled and thus the [forward] request, which the Extension attached is not executed. Wallet Owner celebrates 💰 💵 Extension Owner is devastated 😢 😭

Suggested fix

A) extension not found => throw

throw_unless(error::extension_not_found, extension_found);

Having pondering about use cases for a while, I can’t immediately think when someone would send you “donations”, which begin with “random” content, which sometimes might happen to be prefix::extension_action. TON blockchain has lots of various smart contracts, many of which don’t quietly accept messages with every single op-code, so this behavior is not expected. If someone wants to make a “donation”, they can do so by sending an empty message, or a message with op==0.

B) extension not found => check query_id

In case for some reason we still want to accept “donations” with “random” op-codes including op == prefix::extension_action, we might add “extra bits” of verification, before rejecting the extension message. Since query_id is not used for anything, it can be used as such bits.

ifnot (extension_found) {
  int query_id = in_msg_body~load_uint(size::query_id);
  throw_if(error::extension_not_found, query_id == expected_extension_query_id);
  return ();
}

(expected_extension_query_id is some constant) This requires 96-bits of coincidence (extremely unlikely) for some “donation” to be unnecessarily rejected.

[e6654321]

Isn't this just a problem of social engineering?

[nns2009]

No social engineering here

The issue is similar in nature to some of the known issues, but unlike those, it is "scalable" (villain earns more, than what (s)he spends on gas) and it actually doesn't require scalability (one successful highjack can be enough).

[tolya-yanot]

In the process of working on this smart contract, we did have to choose between throw or return in recv_internal.

This is a debatable point, each option has pros and cons:

Pros of doing always throw:

• the extension recognises that it has already been deleted.
• it is possible to move light checks seqno, expired before signature checking.
• signed internal recognises that it has invalid binary message (parsing error).

Pros of doing return before authorisation:

• if some other contract (withdrawal in staking for example) use these OPs [low probability]

• users or exchanges will see a false bounced deposit to the wallet and think that the money was received (if there was at least one signed_internal - such a bounce still can be done).

In the end it was considered that an easy way to make a bounced deposit would cause more trouble in practice.

As for the described case:

In most cases deployment and top-up of extension are paid by user's funds (from v5 wallet). So far we haven't been able to think of a case when the extension will send someone else's money to the wallet. Usually, the extensions withdraws money from the wallet on the contrary (e.g. in case of subscriptions).

In your case with ICO proxying - tokens received from ICO will still come to v5 wallet address and extension cannot guarantee their withdrawal. So in the extension relies on the will of the wallet owner in any case. But this can be solved by disabling the signature in favour of the extension.

Thus we cannot take this report as an attack.