Reentrancy vulnerability in GiantMevAndFeesPool.withdrawETH

[code423n4]

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 calls lpTokenETH.burn, then GiantMevAndFeesPool.beforeTokenTransfer, followed by a call to _distributeETHRewardsToUserForToken sends ETH to the user, which allows the user to call any function in the fallback. While GiantMevAndFeesPool.withdrawETH has the nonReentrant modifier, GiantMevAndFeesPool.claimRewards does not have the nonReentrant modifier.
When GiantMevAndFeesPool.claimRewards is called in GiantMevAndFeesPool.withdrawETH, the idleETH is reduced but the ETH is not yet sent to the user, which increases totalRewardsReceived and accumulatedETHPerLPShare, thus making the user receive more rewards when calling GiantMevAndFeesPool.claimRewards.

Proof of Concept

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

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);
}

[dmvt]

Function has the nonReentrant guard.

[c4-judge]

dmvt marked the issue as unsatisfactory:
Invalid

[thereksfour]

@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

code-423n4/2022-01-xdefi-findings#25

[dmvt]

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.

[c4-judge]

dmvt marked the issue as nullified

[c4-judge]

dmvt marked the issue as not nullified

[c4-judge]

dmvt marked the issue as primary issue

[thereksfour]

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

[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