Round Taffy Cottonmouth
High
Due to a logical flaw in the L2Staking.removeStakers(...)
, it will be impossible to remove the last staker in the stakerAddresses
array thus leading to a situation whereby a staker may not be able to process the withdrawal of their funds
When L2Staking.removeStakers(...)
is called to remove a staker,
- if the staker being removed is
- the last element in the
stakerAddresses
array - or the only element in the
stakerAddresses
array,
- the last element in the
the function will revert because the inner loop on L183 attempt to access an element that does not yet exist (an out of bound index) in the stakerAddresses
array
File: L2Staking.sol
173: function removeStakers(address[] calldata remove) external onlyOtherStaking {
174: bool updateSequencerSet = false;
175: for (uint256 i = 0; i < remove.length; i++) {
176: if (stakerRankings[remove[i]] <= latestSequencerSetSize) {
177: updateSequencerSet = true;
178: }
179:
180: if (stakerRankings[remove[i]] > 0) {
181: // update stakerRankings
182: @> for (uint256 j = stakerRankings[remove[i]] - 1; j < stakerAddresses.length - 1; j++) {
183: @> stakerAddresses[j] = stakerAddresses[j + 1];
184: stakerRankings[stakerAddresses[j]] -= 1;
185: }
SNIP .............
202: }
To Illustrate lets assume there are 3 stakers in the stakerAddresses
array (s1
, s2
and s3
with index 0, 1 and 2 respectively)
stakerAddresses.length
= 3- when
s3
was added,stakerRankings[s3]
=stakerAddresses.length
= 3 - remove staker
s3
=>remove.length
= 1
182: @> for (uint256 j = stakerRankings[remove[i]] - 1; j < stakerAddresses.length - 1; j++)
- the loop on L182 starts from
j
= 2 and breaks immediately after the first iteration becausej
< 2 - L183 evaluates to
stakerAddresses[2]
=stakerAddresses[3]
.
The problem is that L183 tries to access an element that is out of bound (in this scenario, there is no element at index 3) in the stakerAddresses
array at this time and as such the call will revert with an array out of bound error
Also, this can prevent sequencers from withdrawing their stake because as shown below and given the preconditions explained above, the slash(...)
call on L1 will revert since it makes a call to _msgRemoveStakers(...)
to remove the slashed sequencer on L2
File: L1Staking.sol
196: function withdraw() external {
197: require(isActiveStaker(_msgSender()), "only active staker");
SNIP .........
210:
211: // send message to remove staker on l2
212: @> _msgRemoveStakers(remove);
213: }
- The staker being removed is the last on the
stakerAddresses
array - or There is only one staker in the
stakerAddresses
array
No response
No response
- This can lead to a situation where a staker's is not able to call
withdraw()
and as such their funds are stuck in theL1Staking
contract without a way to withdraw - This can also block sequencers from getting slashed (because it calls
_msgRemoveStakers(...)
)
No response
Modifiy the L2Staking.removeStakers(...)
function to ensure that the it doesn't try to access an element that is not in the stakerAddresses
array.
File: L2Staking.sol
173: function removeStakers(address[] calldata remove) external onlyOtherStaking {
174: bool updateSequencerSet = false;
175: for (uint256 i = 0; i < remove.length; i++) {
176: if (stakerRankings[remove[i]] <= latestSequencerSetSize) {
177: updateSequencerSet = true;
178: }
179:
180: if (stakerRankings[remove[i]] > 0) {
181: // update stakerRankings
182: for (uint256 j = stakerRankings[remove[i]] - 1; j < stakerAddresses.length - 1; j++) {
-183: stakerAddresses[j] = stakerAddresses[j + 1];
-184: stakerRankings[stakerAddresses[j]] -= 1;
+ if (j != stakerAddresses.length - 1) { // avoid reading the index after the last index
+183: stakerAddresses[j] = stakerAddresses[j + 1];
+184: stakerRankings[stakerAddresses[j]] -= 1;
+ }
185: }
SNIP .............
202: }