itsabinashb - RioLRTOperatorRegistry::exited keys are accounted while calculating unallocated confirmed key

[sherlock-admin3]

itsabinashb

high

RioLRTOperatorRegistry::exited keys are accounted while calculating unallocated confirmed key

Summary

Exited validator keys are accounted while calculating confirmed keys which have not received a deposit yet results less eth deposit to EigenLayer.

Vulnerability Detail

Lets see the OperatorValidatorDetails struct: https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/interfaces/IRioLRTOperatorRegistry.sol#L40-L56 Here exited is part of deposited, when assets are withdrawn the exited is increased. We can see how activeDeposits are calculated: https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTOperatorRegistry.sol#L417 The validators.exited are not active keys, that is why it was removed from validators.deposited, so we can say that exited is a part of deposited but it is inactive part so we had to cut it from deposited to get active part. This inactive part should not be accounted while calculating confirmed active keys which has not received a deposit yet i.e unallocatedConfirmedKeys: https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTOperatorRegistry.sol#L431 But in this above snippet we can see that the inactive part was accounted.

Lets assume following assumptions: validators.total = 50 validators.confirmed = 30 validators.deposited = 10 validators.exited = 5 Unconfirmed keys are = (50 - 30) = 20, we are not interested in this. We are interested in confirmed keys, which contains confirmed number of keys which received deposit and these keys [ which received deposit ] contains confirmed number of keys which exited means not active. As per current implementation of code the exited keys were accounted, if we follow current implementation we can see:

• activeDeposits = validators.deposited - validators.exited = 10 - 5 = 5
• unallocatedConfirmedKeys = validators.confirmed - validators.deposited = 30 - 10 = 20

But if we remove the inactive keys i.e the exited keys then we can see:

• unallocatedConfirmedKeys = validators.confirmed - activeDeposits = 30 - 5 = 25

Each key is chunk of 32 ETH so for first case total amount of ether = 20 x 32 = 640 ether And for second case the amount of ether = 25 x 32 = 800 ether

As we can only allocate up to the number of unallocated confirmed keys so this amount of ether is supposed to sent to EigenLayer.

Impact

Less amount of ether than desired amount will be sent to EigenLayer, see the Vulnerability details section.

Code Snippet

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/main/rio-sherlock-audit/contracts/restaking/RioLRTOperatorRegistry.sol#L412-L463

Tool used

Manual Review

Recommendation

Calculate unallocatedConfirmedKeys like this:

unallocatedConfirmedKeys = validators.confirmed - activeDeposits;

[nevillehuang]

request poc

[sherlock-admin3]

PoC requested from @itsabinashb

Requests remaining: 12

[itsabinashb]

I am trying to create a poc based on above mentioned scenario in my report, but please consult with sponsor about this report, I have shown the problem and pretty sure it is valid.

[solimander]

Invalid - Unallocated confirmed keys are simply the amount of confirmed keys that have not yet received a deposit. It doesn't matter how many of the deposited validators have exited.

[itsabinashb]

Invalid - Unallocated confirmed keys are simply the amount of confirmed keys that have not yet received a deposit. It doesn't matter how many of the deposited validators have exited.

Correct, but if you check few iteration with any value you want you can see in this code :

                  uint256 newDepositAllocation = FixedPointMathLib.min(
                    FixedPointMathLib.min(validators.cap - activeDeposits, unallocatedConfirmedKeys), remainingDeposits
                );                  

every time unallocatedConfirmedKeys is the variable which will be minimum & it is set to newDepositAllocation variable which later is denoted as deposit. So ultimately it affects as i shown in the report.

[itsabinashb]

@solimander @nevillehuang

Assume this: There are 3 operator in heap, operator 1, operator 2 & operator 3.

Operator 1

id = 1 validators.confirmed = 50 validators.deposited = 30 exited = 0

Operator 2

validators.confirmed = 30 validators.deposited = 20 exited = 0

Operator 3

validators.confirmed = 40 validators.deposited = 20 exited = 0 We will assume remainingDeposit = 100, validators.cap = 100 We also assume that : block.timestamp < validators.nextConfirmationTimestamp

Now lets run this while loop:

1st loop

operatorId = 1 activeDeposits = validators.deposited - validators.exited = 30 - 0 = 30 unallocatedConfirmedKey = validators.confirmed - validators.deposited = 50 - 30 = 20 newDepositAllocations = min(min(100 - 30, 20), 100) = 20 [ which is unallocatedConfirmedKeys ] deposited = validators.deposited + newDepositAllocation = 30 + 20 = 50 allocations[ ] = newDepositAllocations remainingDeposits = 100 - 20 = 80

2nd loop

operatorId = 1 activeDeposits = validators.deposited - validators.exited = 50 - 0 = 50 unallocatedConfirmedKey = validators.confirmed - validators.deposited = 50 - 50 = 0 -> as unallocatedConfirmedKey is 0 so this if statement will be executed & operator 1 will be removed from heap and operator 3 i.e operator id 3 will be in 1 st index position of heap. Now we have 2 operators in heap.

3rd loop

operatorId = 3 activeDeposits = validators.deposited - validators.exited = 20 - 0 = 20 unallocatedConfirmedKey = validators.confirmed - validators.deposited = 40 - 20 = 20 newDepositAllocations = min(min(100 - 20, 20), 80) = 20 [ which is unallocatedConfirmedKeys ] deposited = validators.deposited + newDepositAllocation = 20 + 20 = 40 allocations[ ] = newDepositAllocations remainingDeposits = 80 - 20 = 60

4th loop

operatorId = 3 activeDeposits = validators.deposited - validators.exited = 40 - 0 = 40 unallocatedConfirmedKey = validators.confirmed - validators.deposited = 40 - 40 = 0 -> as unallocatedConfirmedKey is 0 so same if statement will be executed as 2nd loop & operator 3 will be removed from heap and operator 2 i.e operator id 2 will be in 1 st index position of heap. Now we have 1 operator in heap.

So we can see every iteration the unallocatedConfirmedKey is the value which sets to newDepositAllocation. And everytime you run the loop you will get the unallocatedConfirmedKey as minimum, so that is how it affects.

[solimander]

This seems to be working as expected. We can only deposit to keys that have confirmed and have not yet received a deposit. If you still believe there's an issue, can you create a POC? That'd be much easier to validate.

[itsabinashb]

This seems to be working as expected. We can only deposit to keys that have confirmed and have not yet received a deposit. If you still believe there's an issue, can you create a POC? That'd be much easier to validate.

I tried to create but i could not implement that scenario, but i believe the mentioned scenario in report is possible and that time the issue will occur. I am sure about it.

[solimander]

@itsabinashb Can you point out exactly what the unexpected behavior is that occurs in your example? As far as I can tell, they need to upload more keys and allow them to confirm. Until they do so, we cannot allocate more funds to them.

[itsabinashb]

@solimander @nevillehuang

Yes you are correct, the code works exactly like this, but the problem is with the way how unallocatedConfirmedKeys is calculated. I just want you to understand that, the amount to be deposited in EigenLayer is the amount of newDepositAllocations which is unAllocatedConfirmedKey, because as i shown in previous example with 3 operators, every time unallocatedConfirmedKeys's value is set to newDepositAllocations. Now please read the issue i explained in report, if the unAllocatedConfirmedKey = 20 then ether will be deposited = 20 32 = 640 & if unAllocatedConfirmedKey = 25 then ether will be deposited = 32 25 = 800. The current implementation of code will calculate unAllocatedConfirmedKey as 20 because the exited keys were accounted, but if we don't account the exited keys then the value of unAllocatedConfirmedKey will be 25 - this is the issue, we should not account the exited keys. Rest of code is fine, working as expected. Please read the report again.

[itsabinashb]

@solimander I hope you understood the bug.

[nevillehuang]

Since watson cannot provide a coded poc, I have to agree with sponsor since I cannot verify his written explanation, and so will leave it to escalation period