Popcorn contest - ulqiorra's results

Lines of code

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 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L274

Vulnerability details

Impact

Reward deltaIndex in _accrueRewards() is multiplied by 10**decimals() but eventually divided by rewards.ONE (which is equal to 10**IERC20Metadata(address(rewardToken)).decimals()) in _accrueUser().

If the number of decimals in MultiRewardEscrow share token differs from the number of decimals in the reward token, then all rewards are multipled by 10 ** (decimals() - rewardToken.decimals()).

Therefore, for example, if an admin adds USDT as the reward token with decimals=6, it will result in the reward for any user to be multiplied by ``10**(18-6) = 1000000000000` on the next block. This will at best lead to a DOS where no one will be able to withdraw funds. But at worst, users will drain the entire reward fund due to inflated calculations in the next block.

Proof of Concept

Put the following test in ./test/ folder and run with forge test --mc DecimalMismatchTest. The test fails because of incorrect supplierDelta calculations:

// SPDX-License-Identifier: GPL-3.0
// 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 DecimalMismatchTest is Test {
  using SafeCastLib for uint256;

  MockERC20 stakingToken;
  MockERC20 rewardToken;
  MultiRewardStaking staking;
  MultiRewardEscrow escrow;

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

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

    // staking token has 18 decimals
    stakingToken = new MockERC20("Staking Token", "STKN", 18);

    // reward token has 6 decimals (for example USDT)
    rewardToken = new MockERC20("RewardsToken1", "RTKN1", 6);

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

    staking = new MultiRewardStaking();
    staking.initialize(IERC20(address(stakingToken)), IMultiRewardEscrow(address(escrow)), address(this));
    
    rewardToken.mint(address(this), 1000 ether);
    rewardToken.approve(address(staking), 1000 ether);

    staking.addRewardToken(
      // rewardToken
      IERC20(address(rewardToken)), 
    
      // rewardsPerSecond
      1e10, 

      // amount
      1e18, 

      // useEscrow
      false, 

      // escrowPercentage
      0, 

      // escrowDuration
      0, 

      // offset
      0
    );

  }

  function testWrongSupplierDelta() public {
    stakingToken.mint(address(bob), 1);
    
    vm.prank(bob);
    stakingToken.approve(address(staking), 1);
    
    vm.prank(bob);
    staking.deposit(1);

    assert (staking.balanceOf(bob) == 1);

    vm.warp(block.timestamp + 1);

    IERC20[] memory a = new IERC20[](1);
    a[0] = IERC20(address(rewardToken));

    vm.prank(bob);

    // 1 second elapsed, so Bob must get a little reward
    // but instead this will REVERT with "ERC20: transfer amount exceeds balance"
    // because the `supplierDelta` is computed incorrect and becomes too large
    staking.claimRewards(bob, a);
  }
}

Tools Used

Manual analysis

Recommended Mitigation Steps

Use the same number of decimals when calculating deltaIndex and supplierDelta.

Lines of code

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

Vulnerability details

Impact

A hacker can drain an ERC-777 reward token funds via reentrancy. This is because in the claimRewards() function, the transfer of the reward token which triggers the hacker's ERC-777 hook takes place before setting accruedRewards[user][_rewardTokens[i]] to zero.

Proof of Concept

The attack scenario:

Step 1. Admin adds an ERC-777 hookable token as the reward token. In such a token, a user can set a callback that will be called every time someone sends money to them.

Step 2. Hacker accumulates rewards for this token and calls claimRewards(hackerAddress, [erc777token]). https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L170

Step 3. The function reads the hacker's reward amount from the contract's storage:

uint256 rewardAmount = accruedRewards[user][_rewardTokens[i]];

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

Step 4. The positive amount of tokens is sent to the hacker. Either on the line https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L182

_rewardTokens[i].transfer(user, rewardAmount)

or on the line https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L200

rewardToken.safeTransfer(user, payout);

Step 5. The ERC-777 hook is triggered. The hacker takes control and calls the claimRewards() function again with the same parameters.

Step 6. In the MultiRewardStaking contract, the hacker's state has not changed, and accruedRewards[user][_rewardTokens[i]] is still equal to its original positive value. Therefore, funds are transferred to the hacker again, and the hook is triggered again, from which they can repeat the attack again.

Step 7. After the hacker repeatedly executes the reentrancy, the code flow finally reaches line 186 and sets the hacker's accrued rewards to zero without any revert. https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/utils/MultiRewardStaking.sol#L186

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

Since no errors occur, the hacker gets a profit.

Tools Used

Manual analysis

Recommended Mitigation Steps

Follow the Check-Effect-Interaction pattern and set the state variable accruedRewards[user][_rewardTokens[i]] to zero before doing an external transfer call.

Lines of code

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

Vulnerability details

Impact

Function _withdraw() can be called from an approved caller to withdraw owner funds. The function accrues rewards for caller and receiver but misses the accrual for owner.

If, for example, the owner didn't accrue any reward from the beginning of time and all staking tokens are withdrawn from the owner by the caller, then the owner will be left with zero shares and his supplierDelta will be calculated as zero. This means that the owner may lose all rewards regardless of how long their stake was active.

Proof of Concept

Put the following test in ./test/ folder and run with forge test --mc MissedAccrueTest. The test fails because Alice's rewards were not accrued:

// SPDX-License-Identifier: GPL-3.0
// 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 MissedAccrueTest is Test {
  using SafeCastLib for uint256;

  MockERC20 stakingToken;
  MockERC20 rewardToken;
  MultiRewardStaking staking;
  MultiRewardEscrow escrow;

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

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

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

    rewardToken = new MockERC20("RewardsToken1", "RTKN1", 18);

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

    staking = new MultiRewardStaking();
    staking.initialize(IERC20(address(stakingToken)), IMultiRewardEscrow(address(escrow)), address(this));
    
    rewardToken.mint(address(this), 1000 ether);
    rewardToken.approve(address(staking), 1000 ether);

    staking.addRewardToken(
      // rewardToken
      IERC20(address(rewardToken)), 
    
      // rewardsPerSecond
      1e10, 

      // amount
      1e18, 

      // useEscrow
      false, 

      // escrowPercentage
      0, 

      // escrowDuration
      0, 

      // offset
      0
    );

  }

  function testMissedAccrual() public {
    assert (stakingToken.balanceOf(bob) == 0);
    assert (stakingToken.balanceOf(alice) == 0);

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

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

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

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

    assert (staking.balanceOf(bob) == 1);
    assert (staking.balanceOf(alice) == 1);

    vm.warp(block.timestamp + 1);

    vm.prank(bob);
    staking.withdraw(1, bob, alice);

    IERC20[] memory a = new IERC20[](1);
    a[0] = IERC20(address(rewardToken));

    staking.claimRewards(bob, a);
    assert (stakingToken.balanceOf(bob) > 0);

    // NOW THIS INCORRECTLY FAILS BECAUSE ALICE ACCRUAL WAS MISSED 
    staking.claimRewards(alice, a);
    assert (stakingToken.balanceOf(alice) > 0);
  }
}

Tools Used

Manual analysis

Recommended Mitigation Steps

Accrue owner in _withdraw() function.

Lines of code

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

Vulnerability details

Impact

The changeRewardSpeed() function computes rewardsEndTimestamp incorrectly for the case block.timestamp < prevEndTime. For example, it may increase the rewardsEndTimestamp by several years forward despite the fact that the fund will be enough for one day only. As a result, some users will claim extra rewards at the expense of other users.

Proof of Concept

The function changeRewardSpeed() calculates rewardsEndTimestamp via

_calcRewardsEnd(
    prevEndTime > block.timestamp ? prevEndTime : block.timestamp.safeCastTo32(),
    rewardsPerSecond,
    remainder
)

If the prevEndTime > block.timestamp then it can be reduced to _calcRewardsEnd(prevEndTime, rewardsPerSecond, remainder) where the remainder is the contract's reward token balance.

Then, in the _calcRewardsEnd() the first condition is met and the amount value is increased from remainder to remainder + rewardsPerSecond * (timeLeft):

if (rewardsEndTimestamp > block.timestamp)
    amount += uint256(rewardsPerSecond) * (rewardsEndTimestamp - block.timestamp);

Thus, the amount becomes larger the original contract's reward token balance and the new rewardsEndTimestamp is calculated incorrectly:

return (block.timestamp + (amount / uint256(rewardsPerSecond))).safeCastTo32();

You can use the following forge test to check for the error. Put the following test in ./test/ folder and run with forge test --mc IncorrectComputation. The test fails because rewardsEndTimestamp calculations are wrong:

// SPDX-License-Identifier: GPL-3.0
// 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";
import { RewardInfo, EscrowInfo } from "../src/interfaces/IMultiRewardStaking.sol";

contract IncorrectComputation is Test {
  using SafeCastLib for uint256;

  MockERC20 stakingToken;
  MockERC20 rewardToken;
  MultiRewardStaking staking;
  MultiRewardEscrow escrow;

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

  uint160 constant rps = 1 ether;
  uint constant funds = 100 ether;

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

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

    rewardToken = new MockERC20("RewardsToken1", "RTKN1", 18);

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

    staking = new MultiRewardStaking();
    staking.initialize(IERC20(address(stakingToken)), IMultiRewardEscrow(address(escrow)), address(this));
    
    rewardToken.mint(address(this), 1000 ether);
    rewardToken.approve(address(staking), 1000 ether);

    staking.addRewardToken(
      // rewardToken
      IERC20(address(rewardToken)), 
    
      // rewardsPerSecond
      rps,

      // amount
      funds, 

      // useEscrow
      false, 

      // escrowPercentage
      0, 

      // escrowDuration
      0, 

      // offset
      0
    );

  }

  function testIncorrectComputation() public {

    stakingToken.mint(address(bob), 1);

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

    (
      /*uint64 ONE1*/,
      /*uint160 rewardsPerSecond1*/,
      uint32 rewardsEndTimestamp1,
      /*uint224 index1*/,
      /*uint32 lastUpdatedTimestamp1*/
    ) = staking.rewardInfos(IERC20(address(rewardToken)));

    assert (rewardsEndTimestamp1 == block.timestamp + funds / rps);

    // now we change the speed to the same number
    // so it shouldn't change anything
    // (but it incorrectly does)
    staking.changeRewardSpeed(IERC20(address(rewardToken)), rps);

    (
      /*uint64 ONE2*/,
      /*uint160 rewardsPerSecond2*/,
      uint32 rewardsEndTimestamp2,
      /*uint224 index2*/,
      /*uint32 lastUpdatedTimestamp2*/
    ) = staking.rewardInfos(IERC20(address(rewardToken)));

    // THIS WILL INCORRECTLY REVERT BECAUSE COMPUTATIONS IN changeRewardSpeed() are wrong!
    assert (rewardsEndTimestamp2 == rewardsEndTimestamp1);
  }
}

Tools Used

Manual analysis

Recommended Mitigation Steps

Fix rewardsEndTimestamp calculations

Instead of implementing custom Owned contract the Open Zeppelin's Ownable2Step could be used: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol

Note that Open Zeppelin has Ownable2StepUpgradeable version too.

---

Consider using the type-aware abi.encodeCall method instead of abi.encodeWithSignature and abi.encodeWithSelector.

---

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

if (caller != owner) _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

the caller must be used instead of msg.sender.