LSD Network - Stakehouse contest - Awesome's results

Lines of code

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

Vulnerability details

Impact

In the function updateNodeRunnerWhitelistStatus its require statement will always fail

Proof of Concept

LiquidStakingManager.sol#L278-L284

278:    function updateNodeRunnerWhitelistStatus(address _nodeRunner, bool isWhitelisted) external onlyDAO {
279:        require(_nodeRunner != address(0), "Zero address");
280:        require(isNodeRunnerWhitelisted[_nodeRunner] != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status");
281:
282:        isNodeRunnerWhitelisted[_nodeRunner] = isWhitelisted;
283:        emit NodeRunnerWhitelistingStatusChanged(_nodeRunner, isWhitelisted);
284:    }

When updateNodeRunnerWhitelistStatus reaches line 280 the require statement check will always revert because the inequality (!=) operator checks whether its two operands are not equal to one another, returning a boolean result and since the left/right operand are both the same the inequality operator will always return false.

block.timestamp is unreliable

Using block.timestamp as part of the time checks could be slightly modified by miners/validators to favor them in contracts that contain logic heavily dependent on them.

Consider this problem and warn users that a scenario like this could occur. If the change of timestamps will not affect the protocol in any way, consider documenting the reasoning and writing tests enforcing that these guarantees will be preserved even if the code changes in the future.

There are 6 instances of this issue:

Line 44-45

Line 44:    lastInteractedTimestamp[_from] = block.timestamp;
Line 45:    lastInteractedTimestamp[_to] = block.timestamp;

Line 96

Line 96:    require(lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp, "Too new");

Line 141

Line 141:    bool isStaleLiquidity = _lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp;

Line 184, Line 230

File: StakingFundsVault.sol

Line 184:    require(_lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Too new");

Line 230:    require(token.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Last transfer too recent");

Line is too long

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 There is 1 instance of this issue:

Line 222

Unspecific Compiler Version Pragma

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.

For example: 🤦 Bad:

pragma solidity ^0.8.0;

🚀 Good:

pragma solidity 0.8.4;

Here are some instances of this issue:

GiantSavETHVaultPool.sol#L1, StakingFundsVault.sol#L3

For more info:

Consensys Audit of 1inch

Solidity docs

Uncheck arithmetics operations that can’t underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible, some gas can be saved by using an unchecked block. There is 1 instance of this issue:

Line 76-89

could be refactored to

        for (uint256 i; i < amountOfTokens;) {
            LPToken token = _lpTokens[i];
            uint256 amount = _amounts[i];

            _assertUserHasEnoughGiantLPToClaimVaultLP(token, amount);

            // Burn giant LP from user before sending them an LP token from this pool
            lpTokenETH.burn(msg.sender, amount);

            // Giant LP tokens in this pool are 1:1 exchangeable with external savETH vault LP
            token.transfer(msg.sender, amount);

            emit LPSwappedForVaultLP(address(token), msg.sender, amount);

            unchecked {
                ++i;
            }
        }

Using Storage Instead of Memory for Structs/Arrays Saves Gas

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declaring the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incurring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.

There are 6 instances of this issue:

Line 324

Line 324:    bytes[] memory keys = new bytes[](1);

Line 805, Line 859, Line 860, Line 893, Line 896

Line 805:    bytes[] memory _blsPublicKeyOfKnots = new bytes[](1);

Line 859:    address[] memory priorityStakers = new address[](0);

Line 860:    bytes[] memory initialKnots = new bytes[](1);

Line 893:    bytes[] memory stakingKeys = new bytes[](1);

Line 896:    uint256[] memory stakeAmounts = new uint256[](1);

Remove Shorthand Addition/Subtraction Assignment

Instead of using the shorthand of addition/subtraction assignment operators (+=, -=) it costs less to remove the shorthand (x += y same as x = x+y) saves ~22 gas

There are 18 instances of this issue:

Line 71, Line 58, Line 97, Line 278, Line 287, Line 185, Line 190, Line 225-227, Line 317, Line 430, Line 511, Line 521, Line 558, Line 658, Line 770, Line 782, Line 839

Store Data in calldata Instead of Memory for Certain Function Parameters

Instead of copying variables to memory, it is typically more cost-effective to load them immediately from calldata. If all you need to do is read data, you can conserve gas by saving the data in calldata. There are 17 instances of this issue:

Line 41, Line 54, Line 69

File: OwnableSmartWallet.sol

Line 41:    function execute(address target, bytes memory callData)

Line 54:    bytes memory callData,

Line 69:    bytes memory callData,

Line 355, Line 360

File: StakingFundsVault.sol

Line 355:    function claimFundsFromSyndicateForDistribution(bytes[] memory _blsPubKeys) external {

Line 360:    function _claimFundsFromSyndicateForDistribution(address _syndicate, bytes[] memory _blsPubKeys) internal {

Line 132-133, Line 337, Line 343, Line 359, Line 472, Line 491, Line 555, Line 583, Line 610, Line 631

File: Syndicate.sol

Line 132:    address[] memory _priorityStakers,

Line 133:    bytes[] memory _blsPubKeysForSyndicateKnots

Line 337:    function updateCollateralizedSlotOwnersAccruedETH(bytes memory _blsPubKey) external {

Line 343:    function batchUpdateCollateralizedSlotOwnersAccruedETH(bytes[] memory _blsPubKeys) external {

Line 359:    function calculateUnclaimedFreeFloatingETHShare(bytes memory _blsPubKey, address _user) public view returns (uint256) {

Line 472:    address[] memory _priorityStakers,

Line 491:    function _updateCollateralizedSlotOwnersLiabilitySnapshot(bytes memory _blsPubKey) internal {

Line 555:    function _registerKnotsToSyndicate(bytes[] memory _blsPubKeysForSyndicateKnots) internal {

Line 583:    function _addPriorityStakers(address[] memory _priorityStakers) internal {

Line 610:    function _deRegisterKnot(bytes memory _blsPublicKey) internal {

Line 631:    bytes memory _blsPublicKey

Line 879

File: LiquidStakingManager.sol

Line 879:    function _autoStakeWithSyndicate(address _associatedSmartWallet, bytes memory _blsPubKey) internal {

Function Order Affects Gas Consumption

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted