search

sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

xiaoming90 - Malicious Users Can Deny Notional Treasury From Receiving Fee #82

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

high

Malicious Users Can Deny Notional Treasury From Receiving Fee

Summary

Malicious users can deny Notional Treasury from receiving fees when rewards are reinvested.

Vulnerability Detail

The claimRewardTokens function will harvest the reward tokens from the Aura Pool, and the reward tokens will be transferred to the Balancer Vault. At lines 77-78, a portion of the reward tokens would be sent to the FEE_RECEIVER. After clarifying with the sponsor, it was understood that the FEE_RECEIVER would be set to Notional Treasury so that it would receive some of the accrued reward tokens.

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L61

File: AuraStakingMixin.sol
61:     function claimRewardTokens() external returns (uint256[] memory claimedBalances) {
62:         uint16 feePercentage = BalancerVaultStorage.getStrategyVaultSettings().feePercentage;
63:         IERC20[] memory rewardTokens = _rewardTokens();
64: 
65:         uint256 numRewardTokens = rewardTokens.length;
66: 
67:         claimedBalances = new uint256[](numRewardTokens);
68:         for (uint256 i; i < numRewardTokens; i++) {
69:             claimedBalances[i] = rewardTokens[i].balanceOf(address(this));
70:         }
71: 
72:         AURA_REWARD_POOL.getReward(address(this), true);
73:         for (uint256 i; i < numRewardTokens; i++) {
74:             claimedBalances[i] = rewardTokens[i].balanceOf(address(this)) - claimedBalances[i];
75: 
76:             if (claimedBalances[i] > 0 && feePercentage != 0 && FEE_RECEIVER != address(0)) {
77:                 uint256 feeAmount = claimedBalances[i] * feePercentage / BalancerConstants.VAULT_PERCENT_BASIS;
78:                 rewardTokens[i].checkTransfer(FEE_RECEIVER, feeAmount);
79:                 claimedBalances[i] -= feeAmount;
80:             }
81:         }
82: 
83:         emit BalancerEvents.ClaimedRewardTokens(rewardTokens, claimedBalances);
84:     }

Within the claimRewardTokens function, it will call the AURA_REWARD_POOL.getReward to harvest the reward tokens. Within the claimRewardTokens function, it also uses the pre-balance and post-balance of the reward tokens to check the actual amount of reward tokens that are transferred into the vault.

However, the issue is that anyone can claim reward tokens from Aura Pool on behalf of any address. Following is the implementation of the getReward function taken from Aura's BaseRewardPool4626 contract called by the vault for reference.

https://etherscan.io/address/0xdcee1c640cc270121faf145f231fd8ff1d8d5cd4

/**
 * @dev Gives a staker their rewards, with the option of claiming extra rewards
 * @param _account     Account for which to claim
 * @param _claimExtras Get the child rewards too?
 */
function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){
    uint256 reward = earned(_account);
    if (reward > 0) {
        rewards[_account] = 0;
        rewardToken.safeTransfer(_account, reward);
        IDeposit(operator).rewardClaimed(pid, _account, reward);
        emit RewardPaid(_account, reward);
    }

    //also get rewards from linked rewards
    if(_claimExtras){
        for(uint i=0; i < extraRewards.length; i++){
            IRewards(extraRewards[i]).getReward(_account);
        }
    }
    return true;
}

modifier updateReward(address account) {
    rewardPerTokenStored = rewardPerToken();
    lastUpdateTime = lastTimeRewardApplicable();
    if (account != address(0)) {
        rewards[account] = earned(account);
        userRewardPerTokenPaid[account] = rewardPerTokenStored;
    }
    _;
}

function earned(address account) public view returns (uint256) {
    return
        balanceOf(account)
            .mul(rewardPerToken().sub(userRewardPerTokenPaid[account]))
            .div(1e18)
            .add(rewards[account]);
}

Assume that a malicious user front runs a call to claim rewards tokens. When a keeper calls the AURA_REWARD_POOL.getReward to harvest the reward tokens, it will return no reward tokens, and therefore the difference between the pre-balance and post-balance of the reward tokens will amount to zero. Therefore, no reward tokens will be sent to the FEE_RECEIVER (Notional Treasury) as a fee.

Proof-of-Concept

The test_claim_rewards_success test case shows that under normal circumstances, the Notional treasury will receive a portion of the accrued BAL and AURA as fees.

The test_claim_rewards_success_frontrun test case shows that if the getReward is front-run by an attacker, the Notional treasury will receive nothing.

The following is the test script and its result. 

import pytest
from brownie import ZERO_ADDRESS, Wei, accounts, interface
from tests.fixtures import *
from tests.balancer.helpers import enterMaturity, get_metastable_amounts
from scripts.common import get_univ3_single_data, get_univ3_batch_data, DEX_ID, TRADE_TYPE

chain = Chain()

def test_claim_rewards_success(StratStableETHstETH):
    (env, vault) = StratStableETHstETH
    primaryBorrowAmount = 100e8
    depositAmount = 50e18
    enterMaturity(env, vault, 1, 0, depositAmount, primaryBorrowAmount, accounts[0])
    chain.sleep(3600 * 24 * 365)
    chain.mine()
    feeReceiver = vault.getStrategyContext()["baseStrategy"]["feeReceiver"]
    feePercentage = vault.getStrategyContext()["baseStrategy"]["vaultSettings"]["feePercentage"] / 1e2
    assert env.tokens["BAL"].balanceOf(vault.address) == 0
    assert env.tokens["AURA"].balanceOf(vault.address) == 0
    assert env.tokens["BAL"].balanceOf(feeReceiver) == 0
    assert env.tokens["AURA"].balanceOf(feeReceiver) == 0

    vault.claimRewardTokens({"from": accounts[1]})

    # Test that the fee receiver received portion of the rewards as fee
    assert env.tokens["BAL"].balanceOf(feeReceiver) > 0
    assert env.tokens["AURA"].balanceOf(feeReceiver) > 0

def test_claim_rewards_success_frontrun(StratStableETHstETH):
    (env, vault) = StratStableETHstETH
    primaryBorrowAmount = 100e8
    depositAmount = 50e18
    enterMaturity(env, vault, 1, 0, depositAmount, primaryBorrowAmount, accounts[0])
    chain.sleep(3600 * 24 * 365)
    chain.mine()
    feeReceiver = vault.getStrategyContext()["baseStrategy"]["feeReceiver"]
    feePercentage = vault.getStrategyContext()["baseStrategy"]["vaultSettings"]["feePercentage"] / 1e2
    assert env.tokens["BAL"].balanceOf(vault.address) == 0
    assert env.tokens["AURA"].balanceOf(vault.address) == 0
    assert env.tokens["BAL"].balanceOf(feeReceiver) == 0
    assert env.tokens["AURA"].balanceOf(feeReceiver) == 0

    auraPool = interface.IAuraRewardPool(vault.getStrategyContext()["stakingContext"]["auraRewardPool"])
    auraPool.getReward(vault.address, True, {"from": accounts[5]}) # Attacker frontrun the getReward
    vault.claimRewardTokens({"from": accounts[1]})

    # Test that the fee receiver received nothing due the frontrunning
    assert env.tokens["BAL"].balanceOf(feeReceiver) == 0
    assert env.tokens["AURA"].balanceOf(feeReceiver) == 0
❯ brownie test tests/balancer/rewards/test_rewards_stable_eth_steth.py --network mainnet-fork
Brownie v1.18.1 - Python development framework for Ethereum

=============================================================================================== test session starts ===============================================================================================
platform linux -- Python 3.8.10, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
plugins: eth-brownie-1.18.1, hypothesis-6.27.3, forked-1.4.0, xdist-1.34.0, web3-5.27.0
collected 2 items                                                                                                                                                                                                 
Attached to local RPC client listening at '127.0.0.1:8545'...

tests/balancer/rewards/test_rewards_stable_eth_steth.py ..                                                                                                                                                  [100%]

========================================================================================== 2 passed, 1 warning in 5.72s ===========================================================================================

Impact

Notional Treasury will not receive a portion of the accrued reward tokens as fees. Loss of assets for Notional protocol and its governance token holders.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/vaults/balancer/mixins/AuraStakingMixin.sol#L61

Tool used

Manual Review

Recommendation

It is recommended not to use the pre-balance and post-balance of the reward tokens when claiming reward tokens. A more robust internal accounting scheme needs to be implemented to keep track of actual reward tokens received from the pool so that the appropriate amount of the accrued reward tokens can be sent to the Notional Treasury.

Reference

A similar high-risk issue was found in the past audit report

jeffywu commented 1 year ago

@weitianjie2000 having some internal accounting seems reasonable here.

T-Woodward commented 1 year ago

Think low severity is reasonable here

Evert0x commented 1 year ago

@T-Woodward why is this a low? Seems like a loss for Notional Treasury and NOTE token holders

T-Woodward commented 1 year ago

Yeah I mean I just don't see it as such a big deal. No loss to user funds. If it started to happen we could just upgrade it out

jeffywu commented 1 year ago

I think given that the CodeArena issue was graded a High, I think Medium is ok as a severity here. I couldn't find the severity guidelines as a reference. The net effect here would be a small loss for the Notional Treasury and NOTE token.