Platform: Code4rena
Start Date: 11/11/2022
Pot Size: $90,500 USDC
Total HM: 52
Participants: 92
Period: 7 days
Judge: LSDan
Total Solo HM: 20
Id: 182
League: ETH
Rank: 32/92
Findings: 1
Award: $475.56
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: 0xSmartContract
Also found by: 0x4non, 0xNazgul, 0xRoxas, 0xdeadbeef0x, 0xmuxyz, 9svR6w, Awesome, Aymen0909, B2, Bnke0x0, CloudX, Deivitto, Diana, Franfran, IllIllI, Josiah, RaymondFam, ReyAdmirado, Rolezn, Sathish9098, Secureverse, SmartSek, Trust, Udsen, a12jmx, aphak5010, brgltd, bulej93, c3phas, ch0bu, chaduke, chrisdior4, clems4ever, cryptostellar5, datapunk, delfin454000, fs0c, gogo, gz627, hl_, immeas, joestakey, lukris02, martin, nogo, oyc_109, pashov, pavankv, peanuts, pedr02b2, rbserver, rotcivegaf, sahar, sakman, shark, tnevler, trustindistrust, zaskoh, zgo
475.5642 USDC - $475.56
| Issue | Contexts | |
|---|---|---|
| LOW‑1 | Missing Checks for Address(0x0) | 1 |
| LOW‑2 | Unused receive() Function Will Lock Ether In Contract | 2 |
| LOW‑3 | IERC20 approve() Is Deprecated | 1 |
| LOW‑4 | Use _safeMint instead of _mint | 2 |
| LOW‑5 | Missing Contract-existence Checks Before Low-level Calls | 12 |
| LOW‑6 | Contracts are not using their OZ Upgradeable counterparts | 56 |
| LOW‑7 | Missing parameter validation in constructor | 4 |
| LOW‑8 | Upgrade OpenZeppelin Contract Dependency | 2 |
| LOW‑9 | The nonReentrant modifier should occur before all other modifiers | 3 |
Total: 83 contexts over 9 issues
| Issue | Contexts | |
|---|---|---|
| NC‑1 | Redundant Cast | 1 |
| NC‑2 | Event Is Missing Indexed Fields | 14 |
| NC‑3 | Public Functions Not Called By The Contract Should Be Declared External Instead | 4 |
| NC‑4 | Implementation contract may not be initialized | 20 |
| NC‑5 | Avoid Floating Pragmas: The Version Should Be Locked | 18 |
| NC‑6 | Unused event may be unused code or indicative of missed emit/logic | 4 |
| NC‑7 | Lines are too long | 18 |
| NC‑8 | Use bytes.concat() | 2 |
| NC‑9 | No need for == true or == false checks | 16 |
Total: 97 contexts over 9 issues
Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.
38: deployer = _deployer;
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/LPToken.sol#L38
Consider adding explicit zero-address validation on input parameters of address type.
receive() Function Will Lock Ether In ContractIf the intention is for the Ether to be used, the function should call another function, otherwise it should revert
629: receive() external payable {}
98: receive() external payable {}
The function should call another function, otherwise it should revert
approve() Is Deprecatedapprove() is subject to a known front-running attack. It is deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead.
https://docs.openzeppelin.com/contracts/3.x/api/token/erc20#IERC20-approve-address-uint256-
870: sETH.approve(syndicate, (2 ** 256) - 1);
Consider using safeIncreaseAllowance() / safeDecreaseAllowance() instead.
_safeMint instead of _mintAccording to openzepplin's ERC721, the use of _mint is discouraged, use _safeMint whenever possible.
https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-_mint-address-uint256-
31: _mint(_recipient, _amount);
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/GiantLP.sol#L31
47: _mint(_recipient, _amount);
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/LPToken.sol#L47
Use _safeMint whenever possible instead of _mint
Low-level calls return success if there is no code present at the specified address.
60: (bool success,) = msg.sender.call{value: _amount}("");
411: (bool transferResult, ) = _recipient.call{value: nodeRunnerAmount}("");
415: (transferResult, ) = dao.call{value: daoAmount}("");
460: (bool result,) = smartWallet.call{value: msg.value}("");
189: (bool result,) = msg.sender.call{value: _amount}("");
209: (bool result,) = _smartWallet.call{value: _amount}("");
190: (bool result,) = msg.sender.call{value: _amount}("");
244: (bool result,) = _wallet.call{value: _amount}("");
67: (bool success, ) = _recipient.call{value: due}("");
78: (bool result, bytes memory message) = target.call{value: value}(callData);
321: (bool success,) = _recipient.call{value: unclaimedUserShare}("");
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L321
668: (bool success,) = _recipient.call{value: unclaimedUserShare}("");
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L668
In addition to the zero-address checks, add a check to verify that <address>.code.length > 0
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.
5: import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
5: import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/GiantLP.sol#L5
5: import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
5: import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 6: import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; 7: import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 8: import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; 9: import { Address } from "@openzeppelin/contracts/utils/Address.sol";
5: import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 6: import { ERC20PermitUpgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol";
5: import { Clones } from "@openzeppelin/contracts/proxy/Clones.sol";
5: import { Clones } from "@openzeppelin/contracts/proxy/Clones.sol";
5: import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 6: import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
5: import { Clones } from "@openzeppelin/contracts/proxy/Clones.sol";
5: import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 6: import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 7: import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
5: import { Clones } from "@openzeppelin/contracts/proxy/Clones.sol";
6: import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; 7: import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; 8: import {Address} from "@openzeppelin/contracts/utils/Address.sol";
5: import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
5: import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; 7: import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; 8: import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; 9: import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L5-L9
5: import { Clones } from "@openzeppelin/contracts/proxy/Clones.sol";
Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.
constructorSome parameters of constructors are not checked for invalid values.
25: pool = _pool;
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/GiantLP.sol#L25
26: transferHookProcessor = ITransferHookProcessor(_transferHookProcessor);
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/GiantLP.sol#L26
15: liquidStakingManager = ILiquidStakingManager(_manager);
17: syndicateImplementation = _syndicateImpl;
Validate the parameters.
An outdated OZ version is used (which has known vulnerabilities, see: https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).
"@openzeppelin/contracts": "^4.5.0"
https://github.com/code-423n4/2022-11-stakehouse/tree/main/package.json#L15
"@openzeppelin/contracts-upgradeabale": "4.5.0"
https://github.com/code-423n4/2022-11-stakehouse/tree/main/package.json#L16
Update OpenZeppelin Contracts Usage in package.json
nonReentrant modifier should occur before all other modifiersCurrently the nonReentrant modifier is not the first to occur, it should occur before all other modifiers.
200: function withdrawETHForStaking( address _smartWallet, uint256 _amount ) public onlyManager nonReentrant returns (uint256) {
239: function withdrawETH(address _wallet, uint256 _amount) public onlyManager nonReentrant returns (uint256) {
255: function unstakeSyndicateSharesForRageQuit( address _sETHRecipient, bytes[] calldata _blsPublicKeys, uint256[] calldata _amounts ) external onlyManager nonReentrant {
Re-sort modifiers so that the nonReentrant modifier occurs first.
The type of the variable is the same as the type to which the variable is being cast
33: address(_deployer)
Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields).
Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
event ETHWithdrawnByDepositor(address depositor, uint256 amount);
event LPTokenBurnt(bytes blsPublicKeyOfKnot, address token, address depositor, uint256 amount);
event NewLPTokenIssued(bytes blsPublicKeyOfKnot, address token, address firstDepositor, uint256 amount);
event LPTokenMinted(bytes blsPublicKeyOfKnot, address token, address depositor, uint256 amount);
event DAOCommissionUpdated(uint256 old, uint256 newCommission);
event DETHRedeemed(address depositor, uint256 amount);
event ETHWithdrawnForStaking(address withdrawalAddress, address liquidStakingManager, uint256 amount);
event CurrentStamp(uint256 stamp, uint256 last, bool isConditionTrue);
event ETHDeposited(address sender, uint256 amount);
event ETHWithdrawn(address receiver, address admin, uint256 amount);
event ERC20Recovered(address admin, address recipient, uint256 amount);
event WETHUnwrapped(address admin, uint256 amount);
event Staked(bytes BLSPubKey, uint256 amount);
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L57
event UnStaked(bytes BLSPubKey, uint256 amount);
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L60
Contracts are allowed to override their parents’ functions and change the visibility from external to public.
function depositETH(uint256 _amount) public payable {
function deployNewLiquidStakingDerivativeNetwork( address _dao, uint256 _optionalCommission, bool _deployOptionalHouseGatekeeper, string calldata _stakehouseTicker ) public returns (address) {
function withdrawETHForStaking( address _smartWallet, uint256 _amount ) public onlyManager nonReentrant returns (uint256) {
function deploySyndicate( address _contractOwner, uint256 _priorityStakingEndBlock, address[] calldata _priorityStakers, bytes[] calldata _blsPubKeysForSyndicateKnots ) public override returns (address) {
OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
19: constructor( address _pool, address _transferHookProcessor, string memory _name, string memory _symbol ) ERC20(_name, _symbol)
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/GiantLP.sol#L19
17: constructor(LSDNFactory _factory)
18: constructor(LSDNFactory _factory)
18: constructor(address _lpTokenImplementation)
41: constructor( address _liquidStakingManagerImplementation, address _syndicateFactory, address _lpTokenFactory, address _smartWalletFactory, address _brand, address _savETHVaultDeployer, address _stakingFundsVaultDeployer, address _optionalGatekeeperDeployer )
14: constructor(address _manager)
14: constructor()
14: constructor()
22: constructor()
16: constructor(address _syndicateImpl)
Avoid floating pragmas for non-library contracts.
While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.
A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.
It is recommended to pin to a concrete compiler version.
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/GiantLP.sol#L1
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/liquid-staking/LPToken.sol#L1
Events that are declared but not used may be indicative of unused declarations where it makes sense to remove them for better readability/maintainability/auditability, or worse indicative of a missing emit which is bad for monitoring or missing logic that would have emitted that event.
121: event CurrentStamp
31: event ERC20Recovered
34: event WETHUnwrapped
45: event CollateralizedSLOTReCalibrated
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L45
Add emit or remove event declaration.
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
222: /// @notice In preparation of a rage quit, restore sETH to a smart wallet which are recoverable with the execution methods in the event this step does not go to plan
98: /// @param _amounts Amount of each LP token that the user wants to burn in exchange for either ETH (if not staked) or dETH (if derivatives minted)
111: /// @param _amounts Amount of each LP token that the user wants to burn in exchange for either ETH (if not staked) or dETH (if derivatives minted)
222: /// @notice Utility function that proxies through to the liquid staking manager to check whether the BLS key ever registered with the network but is now banned
300: // Looking at this contract balance and the ETH that is yet to be transferred from the syndicate, then tell the user how much ETH they have earned
35: /// @dev This contract can be extended to allow lending and borrowing of time slots for borrower to redeem any revenue generated within the specified window
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L35
95: /// @notice Syndicate deployer can highlight addresses that get priority for staking free floating house sETH up to a certain block before anyone can do it
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L95
116: /// @notice Whether a BLS public key, that has been previously registered, is no longer part of the syndicate and its shares (free floating or SLOT) cannot earn any more rewards
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L116
119: /// @notice Once a BLS public key is no longer part of the syndicate, the accumulated ETH per free floating SLOT share is snapshotted so historical earnings can be drawn down correctly
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L119
341: /// @notice For any new ETH received by the syndicate, at the knot level allocate ETH owed to each collateralized owner and do it for a batch of knots
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L341
356: /// @notice Calculate the amount of unclaimed ETH for a given BLS publice key + free floating SLOT staker without factoring in unprocessed rewards
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L356
372: /// @notice Using `highestSeenBalance`, this is the amount that is separately allocated to either free floating or collateralized SLOT holders
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L372
381: /// @notice Preview the amount of unclaimed ETH available for an sETH staker against a KNOT which factors in unprocessed rewards from new ETH sent to contract
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L381
398: /// @notice Preview the amount of unclaimed ETH available for a collatearlized SLOT staker against a KNOT which factors in unprocessed rewards from new ETH sent to contract
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L398
490: /// Given an amount of ETH allocated to the collateralized SLOT owners of a KNOT, distribute this amongs the current set of collateralized owners (a dynamic set of addresses and balances)
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L490
596: /// @dev Business logic for de-registering a set of knots from the syndicate and doing the required snapshots to ensure historical earnings are preserved
https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/syndicate/Syndicate.sol#L596
bytes.concat()Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>))
135: string memory tokenName = string(abi.encodePacked(baseLPTokenName, tokenNumber) 136: string memory tokenSymbol = string(abi.encodePacked(baseLPTokenSymbol, tokenNumber)
Use bytes.concat() and upgrade to at least Solidity version 0.8.4 if required.
== true or == false checksThere is no need to verify that == true or == false when the variable checked upon is a boolean as well.
291: require(isNodeRunnerBanned(msg.sender) == false, "Node runner is banned from LSD network");
328: require(isBLSPublicKeyBanned(_blsPublicKeyOfKnot) == false, "BLS public key has already withdrawn or not a part of LSD network"); 332: require(isNodeRunnerBanned(nodeRunnerOfSmartWallet[associatedSmartWallet]) == false, "Node runner is banned from LSD network");
393: require(isBLSPublicKeyBanned(_blsPubKeys[i]) == false, "BLS public key is banned or not a part of LSD network");
436: require(_isNodeRunnerValid(msg.sender) == true, "Unrecognised node runner"); 437: require(isNodeRunnerBanned(msg.sender) == false, "Node runner is banned from LSD network"); 469: require(isBLSPublicKeyPartOfLSDNetwork(_blsPublicKey) == false, "BLS public key is banned or not a part of LSD network");
541: require(isBLSPublicKeyBanned(blsPubKey) == false, "BLS public key is banned or not a part of LSD network");
589: require(isBLSPublicKeyBanned(_blsPublicKeyOfKnots[i]) == false, "BLS public key is banned or not a part of LSD network");
688: require(isNodeRunnerWhitelisted[_nodeRunner] == true, "Invalid node runner");
64: require(liquidStakingManager.isBLSPublicKeyBanned(_blsPublicKeyOfKnots[i]) == false, "BLS public key is not part of LSD network");
84: require(liquidStakingManager.isBLSPublicKeyBanned(_blsPublicKeyOfKnot) == false, "BLS public key is banned or not a part of LSD network");
79: require(liquidStakingNetworkManager.isBLSPublicKeyBanned(_blsPublicKeyOfKnots[i]) == false, "BLS public key is not part of LSD network");
114: require(liquidStakingNetworkManager.isBLSPublicKeyBanned(_blsPublicKeyOfKnot) == false, "BLS public key is banned or not a part of LSD network");
611: if (isKnotRegistered[_blsPublicKey] == false) revert KnotIsNotRegisteredWithSyndicate(); 612: if (isNoLongerPartOfSyndicate[_blsPublicKey] == true) revert KnotHasAlreadyBeenDeRegistered();
#0 - c4-judge
2022-11-30T14:17:18Z
dmvt marked the issue as grade-a