Open code423n4 opened 1 year ago
Function has the nonReentrant guard.
dmvt marked the issue as unsatisfactory: Invalid
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
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