#39360 [W&A-Insight] getRandomActiveNodes may return inconsistent results
Submitted on Jan 28th 2025 at 14:02:49 UTC by @Franfran for Audit Comp | Shardeum: Ancillaries III
Report ID: #39360
Report Type: Websites and Applications
Report severity: Insight
Target: https://github.com/shardeum/archive-server/tree/itn4
Impacts:
Taking and/modifying authenticated actions (with or without blockchain state interaction) on behalf of other users without any interaction by that user, such as:
Changing registration information
Commenting
Voting
Making trades
Withdrawals, etc.
Description
Brief/Intro
The NodeList.getRandomActiveNodes(...)
function should return random nodes evenly distributed but it is not the case and may break some assumptions if it happens. Some other corectness issues have been identified.
Vulnerability Details
The getRandomActiveNodes
function and the underlying getRandomItemFromArr
function both contains bugs.
The getRandomItemFromArr
is supposed to return an even distribution of random unique elements from the passed array, but this is not true because of the nodeRejectPercentage
variable, and the result may contain duplicates if this is over 0, because it fakes the distribution and breaks the (partial) fisher-yates shuffle. The POC shows that this function may contain duplicates, meaning that the property of the returned elements being unique is wrong.
Additionally, some other corectness issues have been found, which have minimal impact, but is still very important to keep in mind and to fix since these are broken assumptions, and the code should not be built upon those:
Array is sorted by ID rather than by insertion order
When calling getRandomActiveNodes, the function used to get the list of nodes is getActiveList which will return a sorted list of nodes by ID rather than by insertion order. Meaning that because elements in the lower part of the array (smaller ID) won't even be considered as they are within the N_NODE_REJECT_PERCENT
(5% by default).
The nodes list should be sorted by insertion order to select the most fresh nodes.
Should send the full list if exceeding node_count
instead of just one
node_count
instead of just oneIn the getRandomActiveNodes, if the requested node_count
is 1 or less, the function will just return one node. But what is odd is that if it exceeds the node list, it would also only return one. It might seems more correct to just return the full list.
Impact Details
The most important and most severe: will worsen the consensus property of the robustQuery call in syncFromNetworkConfig with the redundancy being faked by the faulty distribution of random nodes. Note that according to the POC, it's only possible to have elements coming twice, never thrice or more.
Will send duplicated requests to nodes in the exitArchiver and sendRefute functions or may only send to one node if the current node list has less than 5 elements.
getCachedNodeList may return duplicate values which may make the archiver respond to
/nodelist
with duplicate nodes. Archivers currently handle that but they might start to consider the peer as being not trustworthy as it's wasting bandwidth if a reputation system is ever put in place.
References
Links attached when applicable.
Proof of Concept
Proof of Concept
Simply run the script with
And observe the output, if it has found a duplicate, it should show something like:
Was this helpful?