code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/LpGauge.sol#L115-L119 https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/StakerVault.sol#L326-L328

Vulnerability details

Impact

A user can claim all of the available governance tokens or prevent any rewards from being claimed in LpGauge.sol if sufficient time is left between deploying the contract and initialising it in the StakerVault.sol contract by calling initalizeLPGauge() OR if a new LPGauge contract is deployed and added to StakerVault using prepareLPGauge.

Inside LPGauge.sol when calling _poolCheckPoint(), the lastUpdated variable is not initalised so defaults to a value of 0, therefore if the user has managed to stake tokens in the StakerVault then the calculated poolStakedIntegral will be very large (as block.timestamp is very large). Therefore a user can mint most current available governance tokens for themselves when they claim their rewards (or prevent any governance tokens from being claimed).

Proof of Concept

LP Gauge and StakerVault contracts are deployed
Before the initializeLpGauge(), user A will stake 1 token with stakeFor() thereby increasing _poolTotalStaked by 1. As the lpgauge address is equal to the zero address, _userCheckPoint() will not be called and poolLastUpdate will remain at 0.
The user can then directly call _userCheckPoint() and be allocated a very large number of shares. This works because poolLastUpdate is 0 but the staked amount in the vault is larger than 0
Once initializeLPGauge() is called, the user can then call claimRewards() and receive a very large portion of tokens or if poolStakedIntegral exceeds the mint limit set by Minter.sol then no one else can claim governance tokens from the lpGauge.

OR

A new LP Gauge contract is deployed and added to the vault using prepareGauge(). Follow steps 2 to 4.

Tools Used

Manual audit

Recommended Mitigation Steps

Initialise poolLastUpdate in the constructor

GalloDaSballo commented 2 years ago

The warden has identified a way to exploit the uninitialized value for the variable poolLastUpdate

Because this is contingent on setup, and the impact is theft of rewards, I believe Medium Severity to be more appropriate

scaraven commented 2 years ago

Hi, correct me if I am wrong but there is a second scenario where this situation can be exploited as outlined in my report. If governance deploy a new LPGauge they must do it through prepareLPGauge() as initializeLpGauge() will revert. This introduces a significant delay (3 days if I'm correct) before the LPGauge can actually be initialised with executeLPGauge(). Therefore a user can easily monitor this contract and call poolCheckpoint() as soon as soon as prepareLPGauge() is called and the contract has no way to prevent this.

GalloDaSballo commented 2 years ago

@scaraven

I don't see any enforced delay in initializeLpGauge

_setConfig bypasses the need for a MIN_DELAY

Meaning the exploit is not practical for the first gauge

For Gauge that needs to be substituted, that will indeed require a 3 day wait period, _poolCheckpoint can only be effective after the first deposit has happened as well, see: https://github.com/code-423n4/2022-05-backd-findings/issues/100 Which effects how initial rewards are calculated.

Due to the math being off it may be necessary to influence uint256 currentRate = inflationManager.getLpRateForStakerVault(address(stakerVault));

This, the need for a second gauge, and the race condition between the first depositor and other depositors are external conditions, leading me to believe that Medium Severity is appropriate