LSD Network - Stakehouse contest - JTJabba's results

Lines of code

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

Vulnerability details

Impact

After a token transfer of GiantMevAndFeesPool's GiantLP, the receiver gets their claimed amount updated to the correct value, but the sender does not. If more than zero tokens were transferred, that amount in the sender's future rewards will be lost, and any transactions that try to distribute ETH rewards to the sender - which is necessary before a balance change - will revert from an underflow until accumulatedETHPerLPShare has increased enough for the sender to have a nonnegative amount due.

Proof of Concept

GiantMevAndFeesPool provides itsself as the transferHookProcessor to its GiantLP so it can distribute rewards first and update amounts claimed on transfers. In afterTokenTransfer it only updates the claimed amount for the receiving account.

/contracts/liquid-staking/GiantMevAndFeesPool.sol#L170-L173:

170    function afterTokenTransfer(address, address _to, uint256) external {
171        require(msg.sender == address(lpTokenETH), "Caller is not giant LP");
172        _setClaimedToMax(_to);
173    }

Before a token transfer, SyndicateRewardsProcessor::_distributeETHRewardsToUserForToken is called in the pre transfer hook for the sender and receiver, which will revert on line 61 if claimed[_user][_token] > (accumulatedETHPerLPShare * balance) / PRECISION

/contracts/liquid-staking/GiantMevAndFeesPool.sol#L146-L167:

146    function beforeTokenTransfer(address _from, address _to, uint256) external {
147        require(msg.sender == address(lpTokenETH), "Caller is not giant LP");
148        updateAccumulatedETHPerLP();
149
150        // Make sure that `_from` gets total accrued before transfer as post transferred anything owed will be wiped
151        if (_from != address(0)) {
152            _distributeETHRewardsToUserForToken(
153                _from,
154                address(lpTokenETH),
155                lpTokenETH.balanceOf(_from),
156                _from
157            );
158        }
159
160        // Make sure that `_to` gets total accrued before transfer as post transferred anything owed will be wiped
161        _distributeETHRewardsToUserForToken(
162            _to,
163            address(lpTokenETH),
164            lpTokenETH.balanceOf(_to),
165            _to
166        );
167    }

/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L51-L73:

51    function _distributeETHRewardsToUserForToken(
52        address _user,
53        address _token,
54        uint256 _balance,
55        address _recipient
56    ) internal {
57        require(_recipient != address(0), "Zero address");
58        uint256 balance = _balance;
59        if (balance > 0) {
60            // Calculate how much ETH rewards the address is owed / due 

// @audit L61 reverts if claimed[_user][_token] > (accumulatedETHPerLPShare * balance) / PRECISION

61            uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];
62            if (due > 0) {
63                claimed[_user][_token] = due;
64
65                totalClaimed += due;
66
67                (bool success, ) = _recipient.call{value: due}("");
68                require(success, "Failed to transfer");
69
70                emit ETHDistributed(_user, _recipient, due);
71            }
72        }
73    }

Recommended Mitigation Steps

Update the sender's claimed amount in the post transfer hook.

    function afterTokenTransfer(address _from, address _to, uint256) external {
        require(msg.sender == address(lpTokenETH), "Caller is not giant LP");
        _setClaimedToMax(_to);
        _setClaimedToMax(_from);
    }

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L28-L53 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L29-L60

Vulnerability details

Impact

GiantMevAndFeesPool and GiantSavEthVaultPool contain a function for depositing ETH for staking into other contracts. To check if contracts are part of the system, both call liquidStakingNetworkManager on a user provided address and check with the systems's LSDNFactory that the returned address is a valid liquid staking network manager. A malicious contract could be created that implements the necessary functions to receive deposits and returns a valid liquid staking network manager address and be used to drain ETH from the GiantMevAndFeesPool and GiantSavEthVaultPool.

Proof of Concept

/contracts/liquid-staking/GiantMevAndFeesPool.sol#L28-L53:

28    function batchDepositETHForStaking(
29        address[] calldata _stakingFundsVault,
30        uint256[] calldata _ETHTransactionAmounts,
31        bytes[][] calldata _blsPublicKeyOfKnots,
32        uint256[][] calldata _amounts
33    ) external {
34        uint256 numOfVaults = _stakingFundsVault.length;
35        require(numOfVaults > 0, "Zero vaults");
36        require(numOfVaults == _blsPublicKeyOfKnots.length, "Inconsistent lengths");
37        require(numOfVaults == _amounts.length, "Inconsistent lengths");
38        for (uint256 i; i < numOfVaults; ++i) {
39            // As ETH is being deployed to a staking funds vault, it is no longer idle
40            idleETH -= _ETHTransactionAmounts[i];
41
42            StakingFundsVault sfv = StakingFundsVault(payable(_stakingFundsVault[i]));
43            require( // @audit only checks sfv returns a correct value, not if it was actually created by a valid factory/deployer
44                liquidStakingDerivativeFactory.isLiquidStakingManager(address(sfv.liquidStakingNetworkManager())),
45                "Invalid liquid staking manager"
46            );
47            // @audit if contract has matching function signature, all funds can be transferred to it
48            sfv.batchDepositETHForStaking{ value: _ETHTransactionAmounts[i] }(
49                _blsPublicKeyOfKnots[i],
50                _amounts[i]
51            );
52        }
53    }

/contracts/liquid-staking/GiantSavETHVaultPool.sol#L29-L60:

29    function batchDepositETHForStaking(
30        address[] calldata _savETHVaults,
31        uint256[] calldata _ETHTransactionAmounts,
32        bytes[][] calldata _blsPublicKeys,
33        uint256[][] calldata _stakeAmounts
34    ) public {
35        uint256 numOfSavETHVaults = _savETHVaults.length;
36        require(numOfSavETHVaults > 0, "Empty arrays");
37        require(numOfSavETHVaults == _ETHTransactionAmounts.length, "Inconsistent array lengths");
38        require(numOfSavETHVaults == _blsPublicKeys.length, "Inconsistent array lengths");
39        require(numOfSavETHVaults == _stakeAmounts.length, "Inconsistent array lengths");
40
41        // For every vault specified, supply ETH for at least 1 BLS public key of a LSDN validator
42        for (uint256 i; i < numOfSavETHVaults; ++i) {
43            uint256 transactionAmount = _ETHTransactionAmounts[i];
44
45            // As ETH is being deployed to a savETH pool vault, it is no longer idle
46            idleETH -= transactionAmount;
47
48            SavETHVault savETHPool = SavETHVault(_savETHVaults[i]);
49            require( // @audit only checks savETHPool returns a correct value, not if it was actually created by a valid factory/deployer
50                liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())),
51                "Invalid liquid staking manager"
52            );
53            // @audit if contract has matching function signature, all funds can be transferred to it
54            // Deposit ETH for staking of BLS key
55            savETHPool.batchDepositETHForStaking{ value: transactionAmount }(
56                _blsPublicKeys[i],
57                _stakeAmounts[i]
58            );
59        }
60    }

Recommended Mitigation Steps

Contracts that are supposed to be part of the system should have their addresses directly verified. The contract's address should be verified by the liquid staking network manager it provides in addition to the address of the provided liquid staking network manager being verified by the LSDNFactory to be part of the system.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L66-L70 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L43-L47

Vulnerability details

Impact

LPToken contains the map lastInteractedTimestamp which maps addresses to timestamps and is updated for from and to addresses after a token transfer. Many operations will check the last interaction time of an address and revert if it's too recent to combat botting and allow time for funds to be used properly as they're moved around. Addresses' timestamps are checked in some functions that transfer LP/ GiantLP tokens, which sometimes need to be called by other system contracts. Because anyone can update the lastInteractedTimestamp for any address, anyone can block system contracts or certain users from calling any functions that use lastInteractedTimestamp to require a cooldown.

Proof of Concept

lastInteractedTimestamp is updated after token transfers for LP tokens.

/contracts/liquid-staking/LPToken.sol#L66-L70:

66    function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
67        lastInteractedTimestamp[_from] = block.timestamp;
68        lastInteractedTimestamp[_to] = block.timestamp;
69        if (address(transferHookProcessor) != address(0)) transferHookProcessor.afterTokenTransfer(_from, _to, _amount);
70    }

/contracts/liquid-staking/GiantLP.sol#L43-L47:

43    function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
44        lastInteractedTimestamp[_from] = block.timestamp;
45        lastInteractedTimestamp[_to] = block.timestamp;
46        if (address(transferHookProcessor) != address(0)) ITransferHookProcessor(transferHookProcessor).afterTokenTransfer(_from, _to, _amount);
47    }

lastInteractedTimestamp is checked in the following functions:

/contracts/liquid-staking/ETHPoolLPFactory.sol#L76-L82:

76    function rotateLPTokens(LPToken _oldLPToken, LPToken _newLPToken, uint256 _amount) public {
...
81        require(_amount <= _oldLPToken.balanceOf(msg.sender), "Not enough balance");
82        require(_oldLPToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Liquidity is still fresh");

This could delay LP tokens from being rotated out of a BLS key that is never staked.

/contracts/liquid-staking/GiantPoolBase.sol#L93-L97:

93    function _assertUserHasEnoughGiantLPToClaimVaultLP(LPToken _token, uint256 _amount) internal view {
94        require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount");
95        require(_token.balanceOf(address(this)) >= _amount, "Pool does not own specified LP");
96        require(lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp, "Too new");
97    }

This is called by GiantPoolBase::withdrawLPTokens and GiantSavETHPool::withdrawDETH.

/contracts/liquid-staking/SavETHVault.sol#L126-L186:

126    function burnLPToken(LPToken _lpToken, uint256 _amount) public nonReentrant returns (uint256) {
...
141        bool isStaleLiquidity = _lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp;
...
186        require(isStaleLiquidity, "Liquidity is still fresh");

/contracts/liquid-staking/StakingFundsVault.sol#L174-L184:

174    function burnLPForETH(LPToken _lpToken, uint256 _amount) public nonReentrant {
...
184        require(_lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Too new");

/contracts/liquid-staking/StakingFundsVault.sol#L199-L230:

199    function claimRewards(
200        address _recipient,
201        bytes[] calldata _blsPubKeys
202    ) external nonReentrant {
...
230            require(token.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Last transfer too recent");

Recommended Mitigation Steps

More specific time limits could be implemented, such as a timestamp for when an account deposited liquidity into a contract that could be checked before it's withdrawn or rotated.