GetCandidates is prone to Denial of Service

[shargon]

If there are 1024 or more candidates registered and the smart contract call GetCandidates a denial of service will occur.

neo/src/neo/SmartContract/Native/NeoToken.cs

Line 340 in 2a64c1c

public (ECPoint PublicKey, BigInteger Votes)[] GetCandidates(DataCache snapshot)

[Jim8y]

A lot of issues are caused by this ToArray() or ToJson(), is that possible to add a maximum size limit to it?

[shargon]

We should limit the return to the first X candidates

[ixje]

We should limit the return to the first X candidates

Isn't the point of this method that you can get ALL candidates. Perhaps we need a GetCandidatesCount() and add a start_position and count (with an upper limit) to GetCandidates.

Alternatively, change the way registration works so you can limit the max stored candidates. E.g. You can register as candidate, you then have t time to collect sufficient votes to get in the top 1024 (assuming the top is filled). If you fail to make the top, your registration will be removed.

[Jim8y]

If we can limit the maximum size of toarray(), then problems caused with toarray will be solved and prevented. If we just do find one solve one, it might be endless.

[roman-khimov]

Perhaps we need a GetCandidatesCount() and add a start_position and count (with an upper limit) to GetCandidates.

System.Iterator.Next? If we're changing this API of course which is a bit weird with mainnet running.

[ixje]

I guess it comes down to being able to update native contracts to begin with.

[devhawk]

Perhaps we need a GetCandidatesCount() and add a start_position and count (with an upper limit) to GetCandidates.

System.Iterator.Next? If we're changing this API of course which is a bit weird with mainnet running.

Updating GetCandidates to return an iterator would not solve the problem entirely. RpcServer only returns the first RpcServerSettings.MaxIteratorResultItems values from the iterator and provides no mechanism for the caller to specify how many results to return or to skip. Using an iterator would effectively limit the caller to getting the first 'MaxIteratorResultItems` which defaults to 100.

More info in neo-project/neo-modules#629 and neo-project/neo-devpack-dotnet#647

[shargon]

I made a possible solution for it, please check it #2686

[erikzhang]

I think returning an iterator is the best solution.

[devhawk]

I think returning an iterator is the best solution.

Agree 100%, as long as we provide a mechanism for RpcClient to iterator to access the full iterated collection, not just first MaxIteratorResultItems

[erikzhang]

MaxIteratorResultItems is configurable. And we can solve the RPC issue in the future.

[shargon]

I think returning an iterator is the best solution.

New syscall?

[devhawk]

MaxIteratorResultItems is configurable. And we can solve the RPC issue in the future.

Not by the caller

[shargon]

I think returning an iterator is the best solution.

For the new syscall is ok, but for the previous one, just limit?

[erikzhang]

but for the previous one, just limit?

Both are fine for me, limit it, or wait until it exceeds 2048 items to automatically throw an exception.