Backd Tokenomics contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L70-L75 https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/BkdLocker.sol#L302-L322

Vulnerability details

Impact

Users can claim more fees than expected if governance migrates current rewardToken again by fault.

Proof of Concept

In the migrate() function, there is no requirement newRewardToken != rewardToken. If this function is called with the same "rewardToken" parameter, "_replacedRewardTokens" will contain the current "rewardToken" also. Then when the user claims fees, "userShares" will be added two times for the same token at L302-L305, L314-L317. It's because "curRewardTokenData.userFeeIntegrals[user]" is updated at L332 after the "userShares" calculation for past rewardTokens. So the user can get paid more fees than he should.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

You need to add this require() at L71.

require(newRewardToken != rewardToken, Error.SAME_AS_CURRENT);

Lines of code

https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L98-L102 https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L342-L346 https://github.com/code-423n4/2022-05-backd/tree/main/protocol/contracts/StakerVault.sol#L391-L395

Vulnerability details

Impact

StakerVault.unstake(), StakerVault.unstakeFor() would revert with a uint underflow error of StakerVault.strategiesTotalStaked, StakerVault._poolTotalStaked.

Proof of Concept

Currently it saves totalStaked for strategies and non-strategies separately. uint underflow error could be occured in these cases.

Scenario 1.

1. Address A(non-strategy) stakes some amount x and it will be added to StakerVault_poolTotalStaked.
2. This address A is approved as a strategy by StakerVault.inflationManager.
3. Address A tries to unstake amount x, it will be deducted from StakerVault.strategiesTotalStaked because this address is a strategy already. Even if it would succeed for this strategy but it will revert for other strategies because StakerVault.strategiesTotalStaked is less than correct staked amount for strategies.

Scenario 2. There is a transfer between strategy and non-strategy using StakerVault.transfer(), StakerVault.transferFrom() functions. In this case, StakerVault.strategiesTotalStaked and StakerVault._poolTotalStaked must be changed accordingly but there is no such logic.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

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;

}

2. 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; } }

3. 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; } }

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; }

2. 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;     } }

3. 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;     } }

Summary

It was easy to understand the logic for me because the codes have detailed explanations. I recommended adding some more require() for better performance.

Low Risk Issues

1. Add additional require() for better performance.

i) contracts\BkdLocker.sol#L49 require(_govToken != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED); require(_rewardToken != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);

ii) contracts\BkdLocker.sol#L60 "maxBoost" must be greater than "startBoost", otherwise L276-L278 will be revoked. require(maxBoost >= startBoost, Error.INVALID_AMOUNT);

iii) contracts\BkdLocker.sol#L60 "increasePeriod" must be positive, otherwise L276-L278 will be revoked. require(increasePeriod != 0, Error.INVALID_AMOUNT);

iv) contracts\BkdLocker.sol#L123 "amount" must be positive, otherwise the user can prepare unlock endlessly. require(amount != 0, Error.INVALID_AMOUNT);

v) contracts\BkdLocker.sol#L215 require(claimable != 0, Error.INVALID_AMOUNT);

vi) contracts\tokenomics\VestedEscrow.sol#L60 In this setFundAdmin() function at L75, it checks the first require() already. require(fundadmin_ != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED); require(rewardToken_ != address(0), , Error.ZERO_ADDRESS_NOT_ALLOWED);

2. Wrong comment contracts\BkdLocker.sol#L87

There are no "deposit" or "depositFor" functions in this contract. You need to write "lock" or "lockFor" instead.

3. executeKeeperPoolWeight() and batchExecuteKeeperPoolWeights() functions in InflationManager.sol must have some role restrictions. There are 2 other functions. executeLpPoolWeight(), executeAmmTokenWeight().

contracts\tokenomics\InflationManager.sol#L151 contracts\tokenomics\InflationManager.sol#L241 contracts\tokenomics\InflationManager.sol#L326

With the above functions, the batch functions have the modifier "onlyRoles2(Roles.GOVERNANCE, Roles.INFLATION_MANAGER)". I think the above functions should have the same modifier also.

4. claim() function in VestedEscrow.sol should have nonReentrant modifier same as other claim() function at L138.

contracts\tokenomics\VestedEscrow.sol#L113-L115

Otherwise, you can change the function like this(same as VestedEscrowRevocable.sol).

function claim() external virtual override {     claim(msg.sener); }

1. Don't change storage variable needlessly contracts\BkdLocker.sol#L144 Currently it replaces stashedWithdraws[i] even if "i" equals to "stashedWithdraws.length - 1". And it calculates stashedWithdraws.length every time after pop right, you can reduce gas by changing the length variable. You can replace L144 like below.

--length; if(i != length) {     stashedWithdraws[i] = stashedWithdraws[length]; }

2. Needless conditions. contracts\tokenomics\InflationManager.sol#L575 contracts\tokenomics\InflationManager.sol#L589 contracts\tokenomics\InflationManager.sol#L602

From the logic of the contract, "totalKeeperPoolWeight" will be always non-negative. Even if this value could be negative, it will be revoked with underflow error before.

3. Use != 0 instead of > 0 for uint variables contracts\BkdLocker.sol#L91 contracts\BkdLocker.sol#L92 contracts\BkdLocker.sol#L137 contracts\BkdLocker.sol#L139 contracts\BkdLocker.sol#L254 contracts\BkdLocker.sol#L301 contracts\tokenomics\AmmGauge.sol#L88 contracts\tokenomics\AmmGauge.sol#L104 contracts\tokenomics\AmmGauge.sol#L125 contracts\tokenomics\AmmGauge.sol#L147 contracts\tokenomics\FeeBurner.sol#L117 contracts\tokenomics\KeeperGauge.sol#L140 contracts\tokenomics\LpGauge.sol#L68 contracts\tokenomics\LpGauge.sol#L114 contracts\tokenomics\VestedEscrow.sol#L84 contracts\RewardHandler.sol#L63

4. Change storage to memory if possible contracts\BkdLocker.sol#L251

5. An array’s length should be cached to save gas in for-loops contracts\StakerVault.sol#L259 contracts\tokenomics\FeeBurner.sol#L56 contracts\tokenomics\InflationManager.sol#L116 contracts\tokenomics\VestedEscrow.sol#L94 contracts\zaps\PoolMigrationZap.sol#L22 contracts\zaps\PoolMigrationZap.sol#L39

6. Usage of unchecked can reduce the gas cost contracts\StakerVault.sol#L387

allowances[src][msg.sender] = allowance.uncheckedSub(unstaked);