Session 5 Audit Report Review

[Robert-H-Leonard]

Audit report for this discussion is here on the Blockswap protocol: https://code4rena.com/reports/2023-01-blockswap-fv

Please leave a comment with your analysis of a vulnerability and/or questions and thoughts your have.

[thor4]

[M-01]

Impact

DOS when using addPriorityStakers with two consecutive addresses badly ordered (address a2 < address a1), which can be quite frequent (50% chance).

Line 626 in the following code reverts if one hexadecimal address is less than another:

623:         for (uint256 i; i < numOfStakers; ++i) {
624:             address staker = _priorityStakers[i];
625: 
626:             if (i > 0 && staker < _priorityStakers[i-1]) revert DuplicateArrayElements(); 
627: 
628:             isPriorityStaker[staker] = true; 
629: 
630:             emit PriorityStakerRegistered(staker);
631:         }

This makes no sense as addresses are randomly generated and their comparison is arbitrary. As the reviewer points out, this will result in a 50% chance of reverting each time. They point out the devs probably wanted to compare the index of the staker, but that isn't possible in Solidity.
Their recommended fix is to simply remove the offending line:

File: Syndicate.sol
623:         for (uint256 i; i < numOfStakers; ++i) {
624:             address staker = _priorityStakers[i];
625: 
- 626:             if (i > 0 && staker < _priorityStakers[i-1]) revert DuplicateArrayElements(); 
627: 
628:             isPriorityStaker[staker] = true; 
629: 
630:             emit PriorityStakerRegistered(staker);
631:         }

The devs responded by saying they would remove and instead do the following:

check isPriorityStaker[staker] and revert if true

The reviewer replied that this would cost everyone more in gas because storage read operations are expensive and requested that the devs not do this. They chose not to reply again, however.