User lose his remaining rewards in GiantMevAndFeesPool when new deposits happen because _onDepositETH() set claimed[][] to max without transferring user remaining rewards

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L195-L204 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L33-L48

Vulnerability details

Impact

When depositETH() is called in giant pool it calls _onDepositETH() which calls _setClaimedToMax() to make sure new ETH stakers are not entitled to ETH earned by but this can cause users to lose their remaining rewards when they deposits. code should first transfer user remaining rewards when deposit happens.

Proof of Concept

This is depositETH() code in GiantPoolBase:

    /// @notice Add ETH to the ETH LP pool at a rate of 1:1. LPs can always pull out at same rate.
    function depositETH(uint256 _amount) public payable {
        require(msg.value >= MIN_STAKING_AMOUNT, "Minimum not supplied");
        require(msg.value == _amount, "Value equal to amount");

        // The ETH capital has not yet been deployed to a liquid staking network
        idleETH += msg.value;

        // Mint giant LP at ratio of 1:1
        lpTokenETH.mint(msg.sender, msg.value);

        // If anything extra needs to be done
        _onDepositETH();

        emit ETHDeposited(msg.sender, msg.value);
    }

As you can see it increase user lpTokenETH balance and then calls _onDepositETH(). This is _onDepositETH() and _setClaimedToMax() code in GiantMevAndFeesPool contract:

    /// @dev On depositing on ETH set claimed to max claim so the new depositor cannot claim ETH that they have not accrued
    function _onDepositETH() internal override {
        _setClaimedToMax(msg.sender);
    }

    /// @dev Internal re-usable method for setting claimed to max for msg.sender
    function _setClaimedToMax(address _user) internal {
        // New ETH stakers are not entitled to ETH earned by
        claimed[_user][address(lpTokenETH)] = (accumulatedETHPerLPShare * lpTokenETH.balanceOf(_user)) / PRECISION;
    }

As you can see the code set claimed[msg.sender][address(lpTokenETH] to maximum value so the user wouldn't be entitled to previous rewards but if user had some remaining rewards in contract he would lose those rewards can't withdraw them. these are the steps: 1- user1 deposit 10 ETH to giant pool and accumulatedETHPerLPShare value is 2 and claimed[user1][lpTokenETH] would be 10 * 2 = 20. 2- some time passes and accumulatedETHPerLPShare set to 4 and user1 has 10 * 4 - 20 = 20 unclaimed ETH rewards (the formula in the code: balance * rewardPerShare - claimed). 3- user deposit 5 ETH to giant pool and accumulatedETHPerLPShare is 4 so the code would call _onDepositETH() which calls _setClaimedToMax which sets claimed[user1][lpTokenETH] to 15 * 4 = 60. 4- user1 new remaining ETH reward would be 15 * 4 - 60 = 0. and user1 won't receive his rewards because when he deposits contract don't transfer remaining rewards and set claim to max so user loses his funds.

Tools Used

VIM

Recommended Mitigation Steps

when deposit happens contract should first send remaining rewards, then increase the user's balance and then set the user claim to max.

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

vince0656 marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as satisfactory

[c4-judge]

dmvt marked the issue as selected for report

[5z1punch]

I dont konw if its valide to raise a question after sponsor confimed. I have found the same issue during the contest. But I finally denied myself when I wrote a test for it. The point is the giantFeesAndMevPool will send the rewards when the user deposit eth to it at the second time and after that, the accumulatedETHPerLPShare will be updated. So there is no losses.

I modified my test file at the time according to the instructions for this issue, hoping to help the judge. test/foundry/CanIClaim.t.sol

pragma solidity ^0.8.13;

// SPDX-License-Identifier: MIT
import {GiantPoolTests} from "./GiantPools.t.sol";
import "forge-std/console.sol";

contract CanIClaimTest is GiantPoolTests {
    function test_canIclaim() public{
        // 0. let accumulatedETHPerLPShare value = 2
        address feesAndMevUserZero = accountOne; vm.deal(feesAndMevUserZero, 100 ether);
        vm.startPrank(feesAndMevUserZero);
        giantFeesAndMevPool.depositETH{value: 1 ether}(1 ether);
        vm.stopPrank();
        address(giantFeesAndMevPool).call{value: 2 ether}("");
        giantFeesAndMevPool.updateAccumulatedETHPerLP();
        console.log("0-accumulatedETHPerLPShare", giantFeesAndMevPool.accumulatedETHPerLPShare() / giantFeesAndMevPool.PRECISION());
        // 1- user1 deposit 10 ETH to giant pool and accumulatedETHPerLPShare value is 2 and claimed[user1][lpTokenETH] would be 10 * 2 = 20.
        uint user1_origin_balance = 100 ether;
        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, user1_origin_balance);
        vm.startPrank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 10 ether}(10 ether);
        vm.stopPrank();
        uint256 user_claimed = giantFeesAndMevPool.claimed(feesAndMevUserOne, address(giantFeesAndMevPool.lpTokenETH()));
        console.log("1-user1 claimed",user_claimed / 1e18);
        // 2- some time passes and accumulatedETHPerLPShare set to 4 and user1 has 10 * 4 - 20 = 20 unclaimed ETH rewards (the formula in the code: balance * rewardPerShare - claimed).
        address(giantFeesAndMevPool).call{value: 22 ether}("");
        giantFeesAndMevPool.updateAccumulatedETHPerLP();
        console.log("2-accumulatedETHPerLPShare", giantFeesAndMevPool.accumulatedETHPerLPShare() / giantFeesAndMevPool.PRECISION());
        // 3- user deposit 5 ETH to giant pool and accumulatedETHPerLPShare is 4 so the code would call _onDepositETH() which calls _setClaimedToMax which sets claimed[user1][lpTokenETH] to 15 * 4 = 60.
        vm.startPrank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 5 ether}(5 ether);
        vm.stopPrank();
        // here you can see user1 has already receive the rewards when he depositETH at the second time.
        console.log("3-user1 balance", feesAndMevUserOne.balance);
        user_claimed = giantFeesAndMevPool.claimed(feesAndMevUserOne, address(giantFeesAndMevPool.lpTokenETH()));
        console.log("3-user1 claimed",user_claimed / 1e18);
        // !!! note here !!!
        // 4- assert
        console.log("now user1 balance", feesAndMevUserOne.balance);
        assertEq(user1_origin_balance - 10 ether - 5 ether + (22 ether * 10 / 11), feesAndMevUserOne.balance);
    }
}

run test

forge test --match-test test_canIclaim -vvv

Result

[PASS] test_canIclaim() (gas: 379033)
Logs:
  0-accumulatedETHPerLPShare 2
  1-user1 claimed 20
  2-accumulatedETHPerLPShare 4
  3-user1 balance 105000000000000000000
  3-user1 claimed 60
  now user1 balance 105000000000000000000

[vince0656]

it indeed was true:

    /// @dev On depositing on ETH set claimed to max claim so the new depositor cannot claim ETH that they have not accrued
    function _onDepositETH() internal override {
        _setClaimedToMax(msg.sender);
    }

    /// @dev Internal re-usable method for setting claimed to max for msg.sender
    function _setClaimedToMax(address _user) internal {
        // New ETH stakers are not entitled to ETH earned by
        claimed[_user][address(lpTokenETH)] = (accumulatedETHPerLPShare * lpTokenETH.balanceOf(_user)) / PRECISION;
    }

_setClaimedToMax(msg.sender); was being called without distributing potential rewards first

[vince0656]

        console.log("1-user1 claimed",user_claimed / 1e18);

this really isn't super clear to me - they have to deposit multiple times? still sounds like a bug

[5z1punch]

it indeed was true:

    /// @dev On depositing on ETH set claimed to max claim so the new depositor cannot claim ETH that they have not accrued
    function _onDepositETH() internal override {
        _setClaimedToMax(msg.sender);
    }

    /// @dev Internal re-usable method for setting claimed to max for msg.sender
    function _setClaimedToMax(address _user) internal {
        // New ETH stakers are not entitled to ETH earned by
        claimed[_user][address(lpTokenETH)] = (accumulatedETHPerLPShare * lpTokenETH.balanceOf(_user)) / PRECISION;
    }

_setClaimedToMax(msg.sender); was being called without distributing potential rewards first

It will call the GiantLP.mint before _setClaimedToMax. And the GiantLP override the beforeTokenTransfer function as the GiantMevAndFeesPool.beforeTokenTransfer. As you can see, the rewards will be distributed in the function before _setClaimedToMax:

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

        // Make sure that `_from` gets total accrued before transfer as post transferred anything owed will be wiped
        if (_from != address(0)) {
            _distributeETHRewardsToUserForToken(
                _from,
                address(lpTokenETH),
                lpTokenETH.balanceOf(_from),
                _from
            );
        }

        // Make sure that `_to` gets total accrued before transfer as post transferred anything owed will be wiped
        _distributeETHRewardsToUserForToken(
            _to,
            address(lpTokenETH),
            lpTokenETH.balanceOf(_to),
            _to
        );
    }

[5z1punch]

        console.log("1-user1 claimed",user_claimed / 1e18);

this really isn't super clear to me - they have to deposit multiple times? still sounds like a bug

As described in the issue, user1 deposited twice. I don't understand your question about the console.log... 1-user1 claimed is claimed[user1][lpTokenETH].

[dmvt]

Ok. I'm going to leave this one in place. Thank you for the additional information @5z1punch. The warden and sponsor agree that the bug is still there regardless of the test you've provided.

[trust1995]

Function is working as intended! lpTokenETH.mint(msg.sender, msg.value) -> beforeTokenTransfer -> _distributeETHRewardsToUserForToken -> update claimed and distributes rewards. Finding is invalid. @GalloDaSballo

[dmvt]

See my response in the post-judging qa discussion.