LSD Network - Stakehouse contest - 0x4non's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L63

Vulnerability details

Impact

The variable claimed that keeps tracking of the total amount claimed per user per token its being being resetting with a wrong value. This impacts on the line due calculation on SyndicateRewardsProcessor.sol#L61

            uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];

Proof of Concept

Yo could create a contract that mint LP, then in one transaction loop a transfer function with no value and use the received ether to redeposit and repeat.

Tools Used

manual revision

Recommended Mitigation Steps

On SyndicateRewardsProcessor.sol#L63 instead of

                claimed[_user][_token] = due;

You could do

                claimed[_user][_token] += due;

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L146 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L152 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L161 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L315 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L333 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L337 https://github.com/code-423n4/2022-11-stakehouse/blob/39a3a84615725b7b2ce296861352117793e4c853/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L67

Vulnerability details

Impact

A malicious user can craft a contract to reject ether loking the transfers.

Proof of Concept

Before and after gMevETH and StakingFundsVault is transfer is using the beforeTokenTransfer to pay user any unclaimed ETH rewards. This is dangerous because any user can be a contract that blocks ETH reception making this funtion fail all the times. Plus its a reentrancy risk.

Tools Used

Manual revision

Recommended Mitigation Steps

Instead of transfering ETH you could wrap ETH in WETH and do a regular ERC20 transfer.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/39a3a84615725b7b2ce296861352117793e4c853/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L67 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L146 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L315

Vulnerability details

Impact

Multiple reentrancy issues mainly trigger by a low call ETH transfer.

Proof of Concept

Everytime you use the _distributeETHRewardsToUserForToken method on SyndicateRewardsProcessor.sol#L67 there are many reentrancy opportunities for an attacker.

For example, a contract can call the function transfer of GiantLP several times to transfer an amount from and to self. Because the update of the claimed would not be done until it is executed the function _afterTokenTransfer, the due amount calculated in _distributeETHRewardsToUserForToken of SyndicateRewardsProcessor contract and the lastInteractedTimestamp of GiantLP contract will be incorrect.

Tools Used

Manual revision

Recommended Mitigation Steps

You could keep adding nonReentrant modifiers, but i think the safest call here is to wrap ETH into WETH an transfer it as a normal ERC20 to avoid unwanted callbacks.

QA

Low

Unresolved TODO that could lead to ether stuck

On Syndicate.sol#L195 you have an unresolved todo that could lead to ETH stuck on the contract.

Non critical

Pragma too open

The codebase uses floating pragma. All contracts should be compiled with same pragma version. Locking the pragma ensures that contracts do not accidentally get deployed using either an outdated buggy compiler version or a compiler version different from what the code has been tested with. Recommendation: use a fixed solc version on contracts, and open progma on interfaces.

Files:

• OptionalGatekeeperFactory.sol
• LPToken.sol
• GiantMevAndFeesPool.sol
• StakingFundsVault.sol
• SavETHVault.sol
• OptionalHouseGatekeeper.sol
• SyndicateRewardsProcessor.sol
• LSDNFactory.sol
• StakingFundsVaultDeployer.sol
• GiantLP.sol
• LiquidStakingManager.sol
• ETHPoolLPFactory.sol
• SavETHVaultDeployer.sol
• GiantSavETHVaultPool.sol
• LPTokenFactory.sol
• GiantPoolBase.sol
• OwnableSmartWallet.sol
• OwnableSmartWalletFactory.sol

Using draft version of Openzeppelin ERC20PermitUpgradeable

On LPToken.sol#L6 you are using draft version of Openzeppelin ERC20PermitUpgradeable. This contract is still a draft and is not considered ready for mainnet use. OpenZeppelin contracts may be considered draft contracts if they have not received adequate security auditing or are liable to change with future development.

Missing a __gap[50] storage variable on Syndicate and LpToken

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

Recommendation: add __gap variable:

+ uint256[50] __gap;

On this contracts;

• LPToken.sol#L21
• Syndicate.sol#L121

Avoid magic numbers

On LiquidStakingManager.sol#L870 instead of the magic number (2 ** 256) - 1 use typeof(uint256).max for approving max; Instead of

        sETH.approve(syndicate, (2 ** 256) - 1);

USe

        sETH.approve(syndicate, typeof(uint256).max);

Un unused custom errors

Error InvalidBLSPubKey is not being in use; SyndicateErrors.sol#L8 Syndicate.sol#L15

Error KnotSlashed is not being in use; SyndicateErrors.sol#L10 Syndicate.sol#L17

Error NotCollateralizedOwnerAtIndex is not being in use; SyndicateErrors.sol#L20 Syndicate.sol#L27

Events that arent emit

There are some events declared that are never emmited, you should remove them or emit them;

• Event ETHDeposited on StakingFundsVault.sol#L25
• Event ERC20Recovered on StakingFundsVault.sol#L31
• Event WETHUnwrapped on StakingFundsVault.sol#L34

Format code using forge fmt or other tool

Use format first license then pragma

Please review the solidity coding convention; https://docs.soliditylang.org/en/v0.8.17/style-guide.html

The usually convention is license first, then pragma.

You could go with this on;

• GiantLP.sol#L1-L3
• GiantMevAndFeesPool.sol#L1-L3
• GiantPoolBase.sol#L1-L3
• LPToken.sol#L1-L3
• LPTokenFactory.sol#L1-L3
• LSDNFactory.sol#L1-L3
• OptionalHouseGatekeeper.sol#L1-L3
• SavETHVaultDeployer.sol#L1-L3
• Syndicate.sol#L1-L3
• SyndicateErrors.sol#L1-L3
• SyndicateFactory.sol#L1-L3
• IGateKeeper.sol#L1-L3
• ILPTokenInit.sol#L1-L3
• ILiquidStakingManager.sol#L1-L3
• ILiquidStakingManagerChildContract.sol#L1-L3
• ISyndicateFactory.sol#L1-L3
• ISyndicateInit.sol#L1-L3
• ITransferHookProcessor.sol#L1-L3