search

code-423n4 / 2022-05-backd-findings

0 stars 0 forks source link

Strategy in StakerVault.sol can steal more rewards even though it's designed strategies shouldn't get rewards. #85

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L95 https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/tokenomics/LpGauge.sol#L52-L63

Vulnerability details

Impact

Strategy in StakerVault.sol can steal more rewards even though it's designed strategies shouldn't get rewards. Also there will be a problem with a rewarding system in LpGauge.sol so that some normal users wouldn't get rewards properly.

Proof of Concept

  1. Strategy A staked amount x and x will be added to StakerVault.strategiesTotalStaked. contracts\StakerVault.sol#L312
  2. Strategy A transferred the amount x to non-strategy B and StakerVault.strategiesTotalStaked, StakerVault._poolTotalStaked won't be updated. contracts\StakerVault.sol#L111
  3. After some time for the larger LpGauge.poolStakedIntegral, B claims rewards using the LpGauge.claimRewards() function. contracts\tokenomics\LpGauge.sol#L52

Inside LpGauge.userCheckPoint(), it's designed not to calculate LpGauge.perUserShare for strategy, but it will pass this condition because B is not a strategy. contracts\tokenomics\LpGauge.sol#L90

Furthermore, when calculate rewards, LpGauge.poolStakedIntegral will be calculated larger than a normal user stakes same amount. It's because StakerVault._poolTotalStaked wasn't updated when A transfers x amount to B so LpGauge.poolTotalStaked is less than correct value. contracts\tokenomics\LpGauge.sol#L113-L117

Finally B can get more rewards than he should and the reward system will pay more rewards than it's designed.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think there will be two methods to fix. Method 1 is to forbid a transfer between strategy and non-strategy so that strategy can't move funds to non-strategy. Method 2 is to update StakerVault.strategiesTotalStaked and StakerVault._poolTotalStaked correctly so that strategy won't claim more rewards than he should even though he claims rewards using non-strategy.

Method 1. You need to modify two functions. StakerVault.transfer(), StakerVault.transferFrom().

  1. You need to add this require() at L112 for transfer(). require(strategies[msg.sender] == strategies[account], Error.FAILED_TRANSFER);

  2. You need to add this require() at L144 for transferFrom(). require(strategies[src] == strategies[dst], Error.FAILED_TRANSFER);

Method 2. I've explained about this method in my medium risk report "StakerVault.unstake(), StakerVault.unstakeFor() would revert with a uint underflow error of StakerVault.strategiesTotalStaked, StakerVault._poolTotalStaked" I will copy the same code for your convenience.

You need to modify 3 functions. StakerVault.addStrategy(), StakerVault.transfer(), StakerVault.transferFrom().

  1. You need to move staked amount from StakerVault._poolTotalStaked to StakerVault.strategiesTotalStaked every time when StakerVault.inflationManager approves a new strategy. You can modify addStrategy() at L98-L102 like this.

function addStrategy(address strategy) external override returns (bool) {     require(msg.sender == address(inflationManager), Error.UNAUTHORIZED_ACCESS);     require(!strategies[strategy], Error.ADDRESS_ALREADY_SET);

    strategies[strategy] = true;     _poolTotalStaked -= balances[strategy];     strategiesTotalStaked += balances[strategy];

    return true; }

  1. You need to add below code at L126 of transfer() function.

if(strategies[msg.sender] != strategies[account]) {     if(strategies[msg.sender]) { // from strategy to non-strategy         _poolTotalStaked += amount;         strategiesTotalStaked -= amount;     }     else { // from non-strategy to strategy         _poolTotalStaked -= amount;         strategiesTotalStaked += amount;     } }

  1. You need to add below code at L170 of transferFrom() function.

if(strategies[src] != strategies[dst]) {     if(strategies[src]) { // from strategy to non-strategy         _poolTotalStaked += amount;         strategiesTotalStaked -= amount;     }     else { // from non-strategy to strategy         _poolTotalStaked -= amount;         strategiesTotalStaked += amount;     } }

chase-manning commented 2 years ago

We think this is a medium risk

GalloDaSballo commented 2 years ago

I believe there's validity to the finding but at the same time I believe the impact is a loss of yield more so than an unfair gain of yield for a strategy.

Specifically the POC is reliant on Depositing as a user, then transferring tokens to a strategy.

I believe this will break the accounting per the POC shown (strategiesTotalStaked will be incorrect)

Then the rewards will be claimable to the strategy as if it were a user, meaning that the extra checks to prevent strategies from gaining staking rewards will be sidestepped.

I believe those tokens will be lost unless all strategies have a way to sweep non-protected tokens.

Because the warden showed how to break accounting, I believe Medium Severity to be valid, that said I don't believe the warden has shown any meaningful economic attack beside end-users losing their own tokens and the rewards attached to them