GVN-01 - High Resource Consumption in candidateList `[High]`

[shargon]

A critical vulnerability related to excessive resource consumption has been identified in the onPersist method of the NeoX Governance contract, which uses the EnumerableSet library. Specifically, the values() method of this library is called multiple times: directly on line 313 and via the _computeConsensus() function on line 325. This method is known for its high gas demand, as it copies all values of the candidateList set in memory, which is described in the OpenZeppelin documentation as potentially expensive.

The main concern is that repeatedly using values() in a method that alters the state of the contract increases the gas cost significantly, especially as the candidateList set grows. This increase in gas consumption not only increases operating costs, but also represents a serious security vulnerability.

Since the registration of new candidates in candidateList is public, an attacker could exploit this feature to register a large number of candidates, causing the execution of onPersist to consume more resources than can be provided in a block, effectively blocking the function and causing a denial of service (DoS).

Remediation:

• Limit the use of values() in critical functions: Redesign the onPersist method to eliminate repeated use of the values() method within operations that change the state of the contract. Consider using iterators or alternative mechanisms that do not require copying the entire set into memory.

• Perform load testing and contract optimization to ensure that key functions can handle a high number of interactions without exceeding resource limits. These tests will help identify and mitigate potential failure points due to excessive gas consumption.

References:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableSet.sol#L211-L218

Source references:

go-ethereum/contracts/solidity/Governance.sol

Line 313 in ca856ce

address[] memory candidates = candidateList.values();

go-ethereum/contracts/solidity/Governance.sol

Line 325 in ca856ce

currentConsensus = _computeConsensus();

[roman-khimov]

onPersist is very special and this call is limited to 30_000_000 gas:

go-ethereum/core/state_processor.go

Line 217 in 7d8b639

_, _, err = vmenv.Call(vm.AccountRef(msg.From), *msg.To, msg.Data, 30_000_000, common.U2560)

If it fails, we're in BIG trouble, all nodes would just stop (and we've seen that in tests, right, @nicolegys?). So it MUST NOT fail. Ideally, it should always use some static known amount of gas irrespective of the state.

Really important thing to fix.

[txhsl]

The repeated use of values() will be removed along with #201. But it is not the operation with the highest cost, because the following _computeConsensus() takes more, may be 20 times higher.

We've used a top K algorithm to save gas, but OnPersist() still takes up to 36,000 gas to handle the list with 20 candidates, when it is going to update the CN list (once per week on T3). With an approximately linear evaluation, it requires about 2100 candidates to hit the current gas limit 30_000_000. With a 1000 GAS register fee cost per address, it takes 2_100_000 GAS to perform this kind of attack.

The sort happens in VM memory is necessary, otherwise we will have to maintain a dynamic list and leave it to voters to pay for. The good news is that we are not calling OnPersist() with a direct transaction but a customized VM invocation, so it will not be included in the block gas limit or affect the traffic, also, the gas limit is configurable. I think we can avoid it by improving the vmenv.Call() settings, what is your opinion?

[roman-khimov]

Gas limit can be raised, true. Maybe we should also limit the number of candidates then. 2K is not a small number for this kind of thing, but we better stop accepting new candidates when we know it can cause problems with onPersist. Then the gas limit can be adjusted, then the candidate limit can be raised (if needed).