Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GiantMevAndFeesPool.withdrawETH distributes too many rewards #239

Closed
code423n4 opened this issue Nov 18, 2022 · 12 comments
Closed

GiantMevAndFeesPool.withdrawETH distributes too many rewards #239

code423n4 opened this issue Nov 18, 2022 · 12 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-129 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L52-L64

Vulnerability details

Impact

GiantMevAndFeesPool.withdrawETH will reduce the value of idleETH before burning GaintLP.
GaintLP.burn will call GiantMevAndFeesPool.beforeTokenTransfer and call updateAccumulatedETHPerLP to calculate accumulatedETHPerLPShare.
Decreasing the value of idleETH before burning GaintLP will cause totalRewardsReceived to increase and accumulatedETHPerLPShare to increase, i.e. treating the ETH deposited by the user as rewards, resulting in distributing too many rewards to the user.

Proof of Concept

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L52-L64
https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L176-L178

Tools Used

None

Recommended Mitigation Steps

Change to

    function withdrawETH(uint256 _amount) external nonReentrant {
        require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount");
        require(lpTokenETH.balanceOf(msg.sender) >= _amount, "Invalid balance");
        require(idleETH >= _amount, "Come back later or withdraw less ETH");

-       idleETH -= _amount;

        lpTokenETH.burn(msg.sender, _amount);
+      idleETH -= _amount;

        (bool success,) = msg.sender.call{value: _amount}("");
        require(success, "Failed to transfer ETH");

        emit LPBurnedForETH(msg.sender, _amount);
    }
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 18, 2022
code423n4 added a commit that referenced this issue Nov 18, 2022
@c4-judge
Copy link
Contributor

dmvt marked the issue as duplicate of #140

@c4-judge
Copy link
Contributor

dmvt marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

dmvt marked the issue as duplicate of #146

@c4-judge
Copy link
Contributor

dmvt marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

dmvt marked the issue as duplicate of #129

@c4-judge
Copy link
Contributor

dmvt marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

dmvt marked the issue as duplicate of #129

@c4-judge
Copy link
Contributor

dmvt marked the issue as partial-25

@c4-judge c4-judge added the partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) label Nov 30, 2022
@c4-judge
Copy link
Contributor

dmvt marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 30, 2022
@dmvt
Copy link

dmvt commented Dec 3, 2022

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.

@thereksfour
Copy link

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

@c4-judge c4-judge removed the partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) label Dec 3, 2022
@c4-judge
Copy link
Contributor

c4-judge commented Dec 3, 2022

dmvt marked the issue as full credit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-129 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants