search

neo-project / neo-node

MIT License
229 stars 224 forks source link

NEP17 Security Alert for Applications #822

Closed vang1ong7ang closed 3 years ago

vang1ong7ang commented 3 years ago

It is necessary for applications to take care about the OnNEP17Payment callback.

some affected applications:

PoC

https://github.com/vang1ong7ang/Tanya/blob/main/PoC/InsufficientFunds.cs

Some Facts

shargon commented 3 years ago

Could you explain the issue? If the issue is that you will pay a lot of if you make an specific call, it solved checking how much gas you will pay before execute it, user will know this amout before send the transaction, isn't it?

And please, use testnet for tests 🙏 ...

roman-khimov commented 3 years ago

Yeah, the GAS amount needed is known well from test execution and any automated systems (like migration) can have some limit on that. If that's the only problem then it's mostly app backend/UI issue.

use testnet for tests :pray: ...

:+1:

BTW, do we need to allow 0 amount transfers? They don't make much sense to me and they can at the moment be done even if one doesn't have any NEO or GAS at all. We recently had a nspcc-dev/neo-go#2169 because of that.

vang1ong7ang commented 3 years ago

Could you explain the issue? If the issue is that you will pay a lot of if you make an specific call, it solved checking how much gas you will pay before execute it, user will know this amout before send the transaction, isn't it?

@shargon yeah, that's why i describe it as a notice for applications, instead of the protocol. at least, neo-cli / neo-gui didn't notice the gas usage now, (try send neo command). it is just a remainder especially for exchanges.

And please, use testnet for tests pray ...

in order to test N3 migration, exchange withdraw, and ..., only mainnet can be used.

i'm afraid that for more and more on chain applications, the only way to verify the PoC is to test it on mainnet. (ethereum client can make a local fork of mainnet easily but not neo3 currently). testing by a good guy is better than no test and better than testing by a bad guy kkkk. believe us we know what is happening and we can restrict the bad affects

vang1ong7ang commented 3 years ago

and imo, the deployed mainnet contract can be tagged as a public contract, for testing such issue for the known applications, we can tell the project team to use such contract to test their automated systems and we can test such issue for exchanges to report it earlier.

vncoelho commented 3 years ago

I think it is not really solved, but at least for the official clients it shows a warning now.