Submitted by Dravee
DOS when using addPriorityStakers with two consecutive addresses badly ordered (address a2 < address a1), which can be quite frequent (50% chance).
Theres the following condition in the _addPriorityStakers function at L626:
File: Syndicate.sol
As we can see here, after the index 0, it will revert if the address at index i is less than the address at index i - 1, which is quite an odd condition. Additionally, the custom error is DuplicateArrayElements, which doesnt match with what the written condition is checking.
When adding, as an example, any 2 addresses as Priority Stakers, whether one address is computed to be greater or less than the previous one shouldnt matter (any permutation of those 2 addresses should enable these 2 addresses to be added as Priority Stakers). We can guess here that the condition was badly implemented, making so that adding a list of Priority Stakers has a 50% chance of failing.
You can try the following on Remix by inputting 2 random addresses and see that this can be true or false depending on the order, hence the 50% chance of failure claim:
The following rule catches it as its unreachable with the bug (original code), and passes without it (suggested remediation):
My guess is that here, the developer wanted to somehow report that the address list contains a mistake.
However, here, even if there were duplicates in the array, this wouldnt change anything regarding the final state (just some gas would be wasted with multiple SSTOREs). Id advise against checking if the value is already set in storage before writing to it, as multiple SLOADs can make the function call quite gas heavy very fast.
The real condition was probably intended to be if the stakers index isnt equal to the current index then revert, but for that youd need a way to fetch an index in an array (like JavaScripts indexOf), which isnt the case in Solidity.
The simplest and best solution here is simply to remove the line:
Again, theres no impact besides wasting gas in adding a Priority Staker multiple times, so this revert shouldnt exist in my opinion
vince0656 (Blockswap) commented:
Dravee (warden) commented:
teryanarmen (Certora) commented:
