Reward distribution logic of the ProtocolRewardsPool and EUSDMiningIncentives contracts are fundamentally wrong, resulting in excess rewards for users

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/ProtocolRewardsPool.sol#L167-L169 https://github.com/code-423n4/2023-06-lybra/blob/main/contracts/lybra/miner/EUSDMiningIncentives.sol#L184-L186

Vulnerability details

Impact

The reward distribution logic of the ProtocolRewardsPool and EUSDMiningIncentives contracts effectively allow a user to mint much more rewards than they should be allowed to. This is possible because, unlike a true implementation of the synthetix staking contract, there is no staking function which records an amount deposited for a user, along with updating the userRewardPerTokenPaid for the user. Instead, stakedOf will just return the current balance of the user (rather than e.g. some amount locked in the contract). Additionally, this also means that in the first call to earned, the userRewardPerTokenPaid for the user will always be 0.

The attack path for exploiting this in both the ProtocolRewardsPool and EUSDMiningIncentives contracts are different, but both have the same root cause. In the ProtocolRewardsPool contract, a user who receives (considered staking in this contract) the same amount of esLBR at a future date will receive the same amount of rewards as a user who gets that esLBR earlier. This fundamentally breaks the intended rewards dynamics.

It's even worse in the EUSDMiningIncentives contract, where this can be exploited to mint an ~inf number of reward tokens. This is done by looping through many different accounts and having each call getReward after borrowing EUSD or PeUSD atomically (and repaying in the same tx).

Proof of Concept

Since there is no true stake function for either of the ProtocolRewardsPool and EUSDMiningIncentives contracts, userRewardPerTokenPaid for a user will always be 0 on the first call. A malicious user can take advantage of this and perform the following attack (here I am referring to the EUSDMiningIncentives contract):

1. using a large amount of collateral, call depositAssetToMint on either a EUSD or PeUSD vault, which is in the pools array of the EUSDMiningIncentives contract
2. call getReward on the EUSDMiningIncentives contract (userRewardPerTokenPaid for this address will be 0, minting all rewards from when the rewards period began)
3. on the same vault contract, call burn and then call withdraw (when done in the same tx, there will be no fees owed)
4. send the collateral to a new address and replay steps 1-3 however many times as desired to mint ~inf rewards

Tools Used

Manual review

Recommended Mitigation Steps

If the ProtocolRewardsPool and EUSDMiningIncentives contracts are intended to use Synthetix-like reward distribution logic, then there should be a function where the user actually stakes their tokens (which also updates the userRewardPerTokenPaid), rather than using the current balance of the user.

Assessed type

Other

[c4-pre-sort]

JeffCX marked the issue as primary issue

[LybraFinance]

1. The _mint() and _repay() functions in the eUSD and peUSD contracts both call the refreshMintReward() function of the configurator contract to update the stakedOf value in the EUSDMiningIncentives contract.

2. The esLBR contract updates the stakedOf value in the ProtocolRewardsPool contract when calling the mint() and burn() functions.

[c4-sponsor]

LybraFinance requested judge review

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient proof