No repeat address check for node delegator queue could cause accounting errors

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L162

Vulnerability details

Impact

The function addNodeDelegatorContractToQueue doesn't check that any of the addresses to be added to nodeDelegatorQueue are unique, meaning the same address could be mistakenly added to the queue multiple times. This would lead to incorrect accounting when calling getTotalAssetDeposits as the asset balance of a contract would be counted twice. This in turn would lead to the rsETH price being returned by getRSETHPrice being too high. This means that any users who deposit into the protocol afterward the issue would receive less than their fair share of rsETH while prior depositors would be able to redeem their rsETH for more than their fair share of assets.

On top of this there is no way for the admins to remove a mistaken address from the nodeDelegatorQueue array so the contract would have to e redeployed to resolve the issue.

Recommended Mitigation Steps

addNodeDelegatorContractToQueue already has a non zero address check so it's recommended that a check is added to ensure an address being added isn't already in the nodeDelegatorQueue array. This could be done by adding a state variable mapping (address => bool) public isNodeDelegatorContract that is then checked and assigned in addNodeDelegatorContractToQueue.

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #36

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b