LSD Network - Stakehouse contest - cccz's results

dmvt marked the issue as duplicate of #140

dmvt marked the issue as not a duplicate

dmvt marked the issue as duplicate of #146

dmvt marked the issue as not a duplicate

dmvt marked the issue as duplicate of #129

dmvt marked the issue as not a duplicate

dmvt marked the issue as duplicate of #129

dmvt marked the issue as partial-25

dmvt marked the issue as satisfactory

There are three other channels involved in this conversation. Individual issues are not the correct place to debate. I am engaging you in those channels and having a conversation with the organization regarding your concerns. Please stop repeating yourself on individual issues in this repo.

Let's say there are 10 lpTokenETH in the contract (idleETH = 10) and 3 ETH in rewards. User A has 5 lpTokenETH and User B has 1 lpTokenETH

User A calls the withdrawETH function to withdraw the staked ETH and the reward, he should have received 5 staked ETH and (13-10) / 10 * 5 =1.5 ETH of the reward. But due to the vulnerability, User A will receive (13-(10-5))/10*5 = 4 ETH reward and 5 ETH when exiting the pool.

And User B will revert when exiting the pool due to the overflow in the _updateAccumulatedETHPerLP function

    function totalRewardsReceived() public view override returns (uint256) {
        return address(this).balance + totalClaimed - idleETH; // @auidt: (13-5-4) + 4 - (5-1) = 4
    }
...
    function _updateAccumulatedETHPerLP(uint256 _numOfShares) internal {
        if (_numOfShares > 0) {
            uint256 received = totalRewardsReceived();
            uint256 unprocessed = received - totalETHSeen; // @auidt: 4 - 8 revert

This results in more rewards for user A and losses for later users

dmvt marked the issue as full credit

dmvt marked the issue as duplicate of #60

dmvt marked the issue as partial-25

dmvt marked the issue as satisfactory

dmvt marked the issue as not a duplicate

dmvt marked the issue as duplicate of #59

JeeberC4 marked the issue as duplicate of #147

Function has the nonReentrant guard.

dmvt marked the issue as unsatisfactory: Invalid

@dmvt https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L56-L79

The nonReentrant modifier does not prevent the reentrancy of other functions without the nonReentrant modifier in the withdrawETH function, my similar finding in xdefi

https://github.com/code-423n4/2022-01-xdefi-findings/issues/25

Ok, fair enough. I still think this is functionally equivalent to your finding in #239, but I'll reopen it and see what the sponsor thinks.

dmvt marked the issue as nullified

dmvt marked the issue as not nullified

dmvt marked the issue as primary issue

Ok, fair enough. I still think this is functionally equivalent to your finding in #239, but I'll reopen it and see what the sponsor thinks.

The functionality is similar, but the risks are different.

In #239 users are only distributed more rewards for themselves when they exit the pool.

But this issue is much higher risk, as the user can call claimRewards for any account in the withdrawETH callback to claim more rewards.

Let's say there are 10 lpTokenETH in the contract (idleETH = 10) and 3 ETH in rewards. User A has two accounts, account a has 5 lpTokenETH and account b has 1 lpTokenETH

Using the vulnerability in #239, account a will receive (13-(10-5))/10*5 = 4 ETH reward and 5 ETH when exiting the pool.

But account b will revert when exiting the pool due to the overflow in the _updateAccumulatedETHPerLP function

    function totalRewardsReceived() public view override returns (uint256) {
        return address(this).balance + totalClaimed - idleETH; // @auidt: (13-5-4) + 4 - (5-1) = 4
    }
...
    function _updateAccumulatedETHPerLP(uint256 _numOfShares) internal {
        if (_numOfShares > 0) {
            uint256 received = totalRewardsReceived();
            uint256 unprocessed = received - totalETHSeen; // @auidt: 4 - 8 revert

And using this vulnerability, account b claims the reward in the callback for account a. Since account a's 5 ETH has not been sent at this point, when account b claims the reward, the following calculation is shown

    function totalRewardsReceived() public view override returns (uint256) {
        return address(this).balance + totalClaimed - idleETH; // @auidt: 9 + 4 - 5 = 8
    }
...
    function _updateAccumulatedETHPerLP(uint256 _numOfShares) internal {
        if (_numOfShares > 0) {
            uint256 received = totalRewardsReceived();
            uint256 unprocessed = received - totalETHSeen; // @auidt: 8 - 8 == 0 no revert

At this point accumulatedETHPerLPShare is still (13 - 5)/ 10 = 0.8 Account b receives 0.8 * 1 = 0.8 eth. The reentrant vulnerability creates more attack surfaces for attackers, which is why I reported the two issues separately

vince0656 marked the issue as sponsor confirmed

dmvt marked the issue as satisfactory

dmvt marked the issue as selected for report

dmvt marked the issue as duplicate of #115

dmvt marked the issue as not a duplicate

dmvt marked the issue as duplicate of #114

dmvt marked the issue as not a duplicate

dmvt marked the issue as duplicate of #114

dmvt marked the issue as nullified

doesn't meet the burden of proof

https://github.com/code-423n4/org/issues/74

Let's say there are 10 lpToken in the StakingFundsVault and 3 ETH in rewards. User A has 5 lpToken

User A calls claimRewards to claim his reward, and he should have received half of the 3 ETH rewards. But since StakingFundsVault treats the user's deposit as a reward, the total reward is 13 ETH instead of 3 ETH, and User A will get 13/2 = 6.5 ETH, so User A will withdraw the deposits of other users as reward.

    function totalRewardsReceived() public virtual view returns (uint256) {
        return address(this).balance + totalClaimed; 
    }

dmvt marked the issue as not nullified

dmvt marked the issue as satisfactory

JeeberC4 marked the issue as duplicate of #255

dmvt marked the issue as duplicate of #190

dmvt marked the issue as partial-50

The warden failed to describe a vulnerability, impact, negative result, or otherwise prove that the issue reported exists.

dmvt marked the issue as unsatisfactory: Insufficient proof

@dmvt https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L56-L71 Before calling updateAccumulatedETHPerLP in the claimRewards function, the latest rewards are claimed from the StakingFundsVault, but this is not done before calling updateAccumulatedETHPerLP in beforeTokenTransfer, which leaves the latest rewards undistributed, resulting in users losing their rewards.

I'll reopen this for the sponsor to evaluate, given your additional description. However, I strongly encourage you to include a walkthrough of the specific steps, a test, or a Alice/Bob example in your proof of concept. A link to 3 lines of code is insufficient proof of your report. I'm not 100% clear on it, but I think this may be a duplicate or subset of issue #178.

dmvt marked the issue as nullified

dmvt marked the issue as not nullified

GiantMevAndFeesPool.beforeTokenTransfer will try to distribute the user's current rewards to the user when transferring GaintLP, but since beforeTokenTransfer will not call StakingFundsVault.claimRewards to claim the latest rewards, thus making the calculated accumulatedETHPerLPShare smaller and causing the user to lose some rewards.

In fact this issue is different from #178, which I also reported, and it is caused by claimed[from] not being updated.

The cause of this issue is due to not fetching rewards from the StakingFundsVault in time.

GiantMevAndFeesPool's rewards need to be fetched by calling StakingFundsVault.claimRewards, as done in GiantMevAndFeesPool.claimRewards.

    function claimRewards(
        address _recipient,
        address[] calldata _stakingFundsVaults,
        bytes[][] calldata _blsPublicKeysForKnots
    ) external {
        uint256 numOfVaults = _stakingFundsVaults.length;
        require(numOfVaults > 0, "Empty array");
        require(numOfVaults == _blsPublicKeysForKnots.length, "Inconsistent array lengths");
        for (uint256 i; i < numOfVaults; ++i) {
            StakingFundsVault(payable(_stakingFundsVaults[i])).claimRewards(
                address(this),
                _blsPublicKeysForKnots[i]
            );
        }

        updateAccumulatedETHPerLP();

But beforeTokenTransfer doesn't do that, which causes the rewards to remain in the StakingFundsVault, and the GiantMevAndFeesPool to have fewer rewards available for distribution, causing the user to lose some rewards

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

vince0656 marked the issue as disagree with severity

vince0656 requested judge review

vince0656 marked the issue as sponsor acknowledged

So the nuance here is that due to contract limitations, users should be encouraged for this specific case to claim rewards before transferring tokens due to the requirement of claim params that the contract wouldn't readily have when executing a transfer. We can document this limitation in detail and encourage users to always claim before transferring the tokens

dmvt marked the issue as satisfactory

I think medium is appropriate for this issue given that we have a loss of funds if the user performs actions out of order.

dmvt marked the issue as selected for report