Attacker can withdraw on behalf of another user

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/ProtocolRewardsPool.sol#L100 https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/ProtocolRewardsPool.sol#L141

Vulnerability details

Impact

An attacker can withdraw LBR on behalf of a user. Although the LBR will be withdrawn to the user, this might not be what the user intended to do. Consider the case where the user wants to restake esLBR after redemption time is complete and is denied of the restaking service. An attacker could potentially DOS and withdraw everyone who stakes (and might want to restake later on) by monitoring function call transactions to the stake function or even monitor the time2fullRedemption public mapping to check their redemption times.

Proof of Concept

https://github.com/code-423n4/2023-06-lybra/blob/7b73ef2fbb542b569e182d9abf79be643ca883ee/contracts/lybra/miner/ProtocolRewardsPool.sol#L100

The withdraw function is public, which means an attacker could enter the address of a user to withdraw LBR to the user. This function updates the last withdrawal time on Line 105 as well.

File: contracts/lybra/miner/ProtocolRewardsPool.sol
100: function withdraw(address user) public {
101:        uint256 amount = getClaimAbleLBR(user);
102:        if (amount > 0) {
103:            LBR.mint(user, amount);
104:        }
105:        lastWithdrawTime[user] = block.timestamp;
106:        emit WithdrawLBR(user, amount, block.timestamp);
107:    }

In case where the user wants to restake after their redemption time is complete, the attacker can monitor the public time2fullRedemption mapping for the user's redemptiom time and call the withdraw function (as soon as it ends) with the respective user's address. What this will do is mint the Claimable LBR to the user and update the last withdrawal time in the public withdraw function to be block.timestamp.

File: contracts/lybra/miner/ProtocolRewardsPool.sol
141: function reStake() external {
142:         uint256 amount = getReservedLBRForVesting(msg.sender) + getClaimAbleLBR(msg.sender);
143:         esLBR.mint(msg.sender, amount);
144:         unstakeRatio[msg.sender] = 0;
145:         time2fullRedemption[msg.sender] = 0;
146:         emit Restake(msg.sender, amount, block.timestamp);
147:     }

So now when the user goes to restake, getClaimableLBR() on Line 142 returns 0 since the if condition fails in getClaimableLBR() i.e. lastWithdrawTime[user] > time2fullRedemption[user].

File: contracts/lybra/miner/ProtocolRewardsPool.sol
155: function getClaimAbleLBR(address user) public view returns (uint256 amount) {
156:         if (time2fullRedemption[user] > lastWithdrawTime[user]) {
157:             amount = block.timestamp > time2fullRedemption[user] ? unstakeRatio[user] * (time2fullRedemption[user] - lastWithdrawTime[user]) : unstakeRatio[user] * (block.timestamp - lastWithdrawTime[user]);
158:        }
159:    }

This form of DOS can lead to the restake function not being utilized correctly since the LBR is always withdrawn, leading to unintentional withdrawal behaviour from the user's end.

Tools Used

Manual Review

Recommended Mitigation Steps

There can be several mitigation steps:

1. Use a require statement to check if msg.sender == user
2. Use msg.sender instead of address user variable in the function
3. Make the withdraw function internal since it is only called from unstake() function

Assessed type

DoS

[c4-pre-sort]

JeffCX marked the issue as primary issue

[c4-pre-sort]

JeffCX marked the issue as duplicate of #364

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[mcgrathcoutinho]

Hi @0xean, This issue is related to a DOS of the restaking service, which the user is denied of by the attacker who calls the withdraw() function. I checked the duplicate tag and its corresponding issue #364 , which is about lastWithdrawTime being updated everytime withdraw() is called. Although the POC might be a bit similar in terms of lastWithdrawalTime update, the impact is different.

Here is the impact difference between both issues:

364 - an attacker can frequently update the lastWithdrawTime variable for any user, effectively denying the user the ability to claim rewards or diluting their rewards.

My finding above - An attacker can withdraw LBR on behalf of a user. Although the LBR will be withdrawn to the user (at end of redemption time), this might not be what the user intended to do. Consider the case where the user wants to restake esLBR after redemption time is complete and is denied of the restaking service.

I would really like to hear from you as to why this was marked as unsatisfactory or invalid.

I appreciate and respect your time for responding to this, thank you!

[0xean]

As far as I understand this "attack", an attacker wastes gas money to withdraw for another user. That other user can simply use the stake call instead of the restake call.

This is a non-sensical attack.