LSD Network - Stakehouse contest - btk's results

Reentrancy attack may occur on withdrawETHForKnot()

Note: It wasn't clear to me whether this should be submitted as high severity or medium severity, so I thought it best to leave it up to the judges.

Lines of code:

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L326-L350

Impact: A reentrancy attack can occur when the contract fails to update its state before the interaction, the attacker can make a recursive call back to the original function in an attempt to drain funds.

Proof of concept: The below scenario would be possible.

Since the node runner could be a smart contract, the contract could have a malicious code so that when withdrawETHForKnot() get called, it goes in infinite loop in attempt to drain all the funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Use a reentrancy guard

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L278-L284

Vulnerability details

Impact

Since the function always revert Due to incorrect require statements, there is no way to add or remove a whitelisted node runner. Therefore, the functions registerBLSPublicKeys that require for a node runner to be whitelisted will also revert.

Proof of Concept

require(isNodeRunnerWhitelisted[_nodeRunner] != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status");

In this require statement you are basically saying that if a node runner is whitelisted then require that is not whitelisted and vice versa.

Tools Used

Manual review

Recommended Mitigation Steps

Fix:

// Replace the incorrect require statement with this one.
require(isWhitelisted != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status");

Gas saving 1: State variables should be cached in memory variables rather than re-reading them from storage

Lines of code: https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L177-L189

State variable:  numberOfRegisteredKnots 

Deployment gas before: 3219394

Deployment gas used after: 3219178

Gas Saved: 216

Lines of code: https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L551

State variable:  totalFreeFloatingShares 

Deployment gas before: 3219394

Deployment gas used after: 3217654

Gas Saved: 1740

We can optimize the above functions by caching the state variables to a memory variables.

Gas saving 2: Add the constant attributes to state variables that never change

Lines of code: https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L158

State variable:  MODULO  

Deployment gas before: 5395557

Deployment gas used after: 5373903

Gas Saved: 21654

Gas saving 3: ++ Cost less gas then += 1

Lines of code: 
https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L782
https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L839

State variable:  numberOfKnots 

Use ++numberOfKnots instead of numberOfKnots += 1

Deployment gas before: 5395557

Deployment gas used after: 5393181

Gas Saved: 2376