nspcc-dev/neofs-contract

Fork 0

mirror of https://github.com/nspcc-dev/neofs-contract.git synced 2026-03-01 04:28:59 +00:00

Check return value of token transfer #69

New issue

Closed

opened 2025-12-28 18:08:25 +00:00 by sami · 4 comments

sami commented

2025-12-28 18:08:25 +00:00

Owner

Originally created by @alexvanin on GitHub (Dec 1, 2021).

Originally assigned to: @alexvanin, @carpawell on GitHub.

I've noticed an issue: alphabet_contract.go (and probably other contracts) has neo.Transfer and gas.Transfer calls. These calls return boolean value whether the transfer was OK. You don't handle this value, it's dangerous.

Originally posted by @AnnaShaleva in https://github.com/nspcc-dev/neofs-contract/pull/185#pullrequestreview-818660980

Originally created by @alexvanin on GitHub (Dec 1, 2021). Originally assigned to: @alexvanin, @carpawell on GitHub. I've noticed an issue: `alphabet_contract.go` (and probably other contracts) has `neo.Transfer` and `gas.Transfer` calls. These calls return boolean value whether the transfer was OK. You don't handle this value, it's dangerous. _Originally posted by @AnnaShaleva in https://github.com/nspcc-dev/neofs-contract/pull/185#pullrequestreview-818660980_

sami

2025-12-28 18:08:25 +00:00

closed this issue
added the
alphabet
label

sami commented

2025-12-28 18:08:26 +00:00

Author

Owner

@alexvanin commented on GitHub (Dec 1, 2021):

There are only 3 places where we ignore return value:

Alphabet contract transfers NEO to itself to produce GAS,
Alphabet contract transfers GAS to proxy contract,
Alphabet contract transfers GAS to storage nodes.

I think it makes sense to check return value only in (1) case. If Alphabet contract can't produce GAS to spend, then it is okay to abort and panic.

But for (2) and (3) I don't think it is worth to do it. If there are some transfer issues (which should never happen, because receivers are not contracts, they are regular wallets, I hope 😃 ) we should not abort but continue GAS emission for other nodes. We can log the failure, but I doubt anyone will see those logs. Maybe produce notification, but idk.

@alexvanin commented on GitHub (Dec 1, 2021): There are only 3 places where we ignore return value: 1. Alphabet contract transfers NEO to itself to produce GAS, 2. Alphabet contract transfers GAS to proxy contract, 3. Alphabet contract transfers GAS to storage nodes. I think it makes sense to check return value only in (1) case. If Alphabet contract can't produce GAS to spend, then it is okay to abort and panic. But for (2) and (3) I don't think it is worth to do it. If there are some transfer issues (which should never happen, because receivers are not contracts, they are regular wallets, I hope :smiley: ) we should not abort but continue GAS emission for other nodes. We can log the failure, but I doubt anyone will see those logs. Maybe produce notification, but idk.

sami commented

2025-12-28 18:08:26 +00:00

Author

Owner

@AnnaShaleva commented on GitHub (Dec 1, 2021):

If there are some transfer issues (which should never happen, because receivers are not contracts, they are regular wallets, I hope)

Transfer can return false not only because of bad receiver, but also when you don't have enough funds to transfer or in case of bad witness or even if token fails to persist storage item changes, so there are various errors that could happen.

I paid attention to this issue in order to ensure it was done intentionally and you're aware of the consequences. If so, then we can keep the old code and omit Transfer result checking.

@AnnaShaleva commented on GitHub (Dec 1, 2021): > If there are some transfer issues (which should never happen, because receivers are not contracts, they are regular wallets, I hope) Transfer can return false not only because of bad receiver, but also when you don't have enough funds to transfer or in case of bad witness or even if token fails to persist storage item changes, so there are various errors that could happen. I paid attention to this issue in order to ensure it was done intentionally and you're aware of the consequences. If so, then we can keep the old code and omit `Transfer` result checking.

sami commented

2025-12-28 18:08:26 +00:00

Author

Owner

@AnnaShaleva commented on GitHub (Dec 1, 2021):

I reviewed carefully usages of transfer, and I think that at least it's worth to add logs if something goes wrong with transfer. It will help to investigate issues.

For case (2) we have the following code:

		gas.Transfer(contractHash, proxyAddr, proxyGas, nil)
		gasBalance -= proxyGas

So here you always assume that transfer succeded and this affects gasBalance variable. This GAS then should be transferred to the storage nodes, so in the bad case when transfer (2) fails, at least proxyGas amount of GAS will still be available on the alphabet contract account, is it OK?

Maybe produce notification, but idk.

I don't think it's worth to produce notifications on failure, these events are not meaningful to anyone except alphabet nodes.

@AnnaShaleva commented on GitHub (Dec 1, 2021): I reviewed carefully usages of transfer, and I think that at least it's worth to add logs if something goes wrong with transfer. It will help to investigate issues. For case (2) we have the following code: ``` gas.Transfer(contractHash, proxyAddr, proxyGas, nil) gasBalance -= proxyGas ``` So here you always assume that transfer succeded and this affects `gasBalance` variable. This GAS then should be transferred to the storage nodes, so in the bad case when transfer (2) fails, at least `proxyGas` amount of GAS will still be available on the alphabet contract account, is it OK? > Maybe produce notification, but idk. I don't think it's worth to produce notifications on failure, these events are not meaningful to anyone except alphabet nodes.

sami commented

2025-12-28 18:08:26 +00:00

Author

Owner

@alexvanin commented on GitHub (Dec 1, 2021):

it's worth to add logs if something goes wrong with transfer.

I am okay with logs, let's do that.

at least proxyGas amount of GAS will still be available on the alphabet contract account, is it OK?

It is. We have a ratio of GAS emission between nodes and proxy contract. It will be more consistent to keep ratio even if some transfers have failed. In next emission, unspent GAS will be reused according to the ratio.

@alexvanin commented on GitHub (Dec 1, 2021): > it's worth to add logs if something goes wrong with transfer. I am okay with logs, let's do that. > at least proxyGas amount of GAS will still be available on the alphabet contract account, is it OK? It is. We have a ratio of GAS emission between nodes and proxy contract. It will be more consistent to keep ratio even if some transfers have failed. In next emission, unspent GAS will be reused according to the ratio.