code-423n4 / 2023-03-neotokyo-findings

4 stars 0 forks source link

Incorrect calculation of rewards, windows problem #121

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1312 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1319 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1376 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1378 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1433 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1434 https://github.com/code-423n4/2023-03-neotokyo/blob/dfa5887062e47e2d0c801ef33062d44c09f6f36e/contracts/staking/NeoTokyoStaker.sol#L1435

Vulnerability details

Title: Incorrect calculation of rewards

Risk rating: High

Impact

The main impact is the incorrect issuance and calculation of the reward amount for the user in a greater or lesser way depending on the conditions. Affects stake S1, S2, LP.

Proof of Concept

Taking into account the names of the tests, answers to questions in discord, and documentation, we conclude: The reward must be distributed according to the following rules:

  1. If there is one reward window per pool then the reward is valid from 0 timestamp until there is a new configuration
  2. If a second window or more appears, then the new amount of reward begins to work from the startTime of the new window. Accordingly, the reward amount of the window is valid from its startTime to the startTime of the next window after it

But in practice, this is not the case

Short

The implementation of the reward calculation algorithm in the method staking/NeoTokyoStaker.sol#getPoolReward and the use of staking/NeoTokyoStaker.sol#RewardWindow is inaccurate with various errors, which leads to the fact that users receive an incorrect reward amount

Example: if User A stakes coins and expects an increase in emission (adding new reward windows) then the reward parameter from the last reward window will be applied for the interval from the moment of the stake to the new user action. This will result in an increased or decreased number of rewards for past window/windows starting from lastPoolRewardTime for the user

Why does this happen?

  1. Complexity of the reward calculation mechanism
  2. The presence of logical errors in it
  3. Documentation mismatch

Proof

An example of a case where the user receives 5.6x more reward than he must receive:

Let's take the following initial settings for simulation and search for a problem, consider a certain ideal case that is easy to understand: The fact that the user's single stake or daoTax - 0 is taken does not affect anything, but it makes it easier to explain the problem

Diagram with calculation and example flow **1**

  1. The administration deployed the contracts and set the initial reward window settings for S2: startTime - 0, reward - 10e18, daoTax - 0

  2. User A stake one S2 Citizen. Time point: May 13 2025, 1747110911 During stake tx calling next method:

    
    stakings/NeoTokyoStaker.sol

1226: IByteContract(BYTES).getReward(msg.sender);

  As a result, since the user did not have stakes before this, no reward is given, and the moment of the last reward request is updated to the value `1747110911`
  ```solidity
stakings/NeoTokyoStaker.sol

1433:       lastRewardTime[_recipient][AssetType.S1_CITIZEN] = block.timestamp;
1434:       lastRewardTime[_recipient][AssetType.S2_CITIZEN] = block.timestamp;
1435:       lastRewardTime[_recipient][AssetType.LP] = block.timestamp;
  1. Use A just waiting

  2. After a certain time, the Administration changes the reward amount (adds a new reward window). Reward windows settings: startTime - 1776227711, reward - 100e18, daoTax - 0, window settings are now as follows

          rewardWindows: [
            {
              startTime: 0,
              reward: ethers.utils.parseEther("10"),
            },
            {
              startTime: 1776227711, // Apr 15 2026 - 1776227711
              reward: ethers.utils.parseEther("100"),
            },
          ],
  3. User A makes a reward withdrawal after a configuration change - time point: May 13 2026, 1778654302

    
    stakings/NeoTokyoStaker.sol

1409: claimReward(User A) ->

1423: (uint256 s2Reward, uint256 s2Tax) = getPoolReward( 1424: AssetType.S2_CITIZEN, 1425: _recipient 1426: ); ->

1264: -> getPoolReward

1309: uint256 totalReward; uint256 lastPoolRewardTime = lastRewardTime[_recipient][_assetType]; # equal May 13 2025, 1747110911 from step 2 uint256 windowCount = pool.rewardCount; # equal 2 for (uint256 i; i < windowCount; ) { # 0 < 2 RewardWindow memory window = pool.rewardWindows[i];

                            # for first iteration 1747110911 < 0 -> FALSE
                            # for second iteration and more 1747110911 < 1776227711 -> FALSE 
            if (lastPoolRewardTime < window.startTime) {
                                      /* ... */ 
                              # for first iteratoin 0 == 2 - 1 -> FALSE
                              # for second iteration 1 == 2 - 1 -> TRUE
            } else if (i == windowCount - 1) { 
                               # the flow in the entire iteration of flow went only here.
                               # the reward that should have been provided for the period of the first window was not calculated for the user
                unchecked {
                                            # timeSinceReward = 1778654302 - 1747110911 = 31,543,391 seconds from last `getReward` for user
                                            # which included the time of window 0 and window 1
                    uint256 timeSinceReward = block.timestamp - lastPoolRewardTime;
                                            # totalReward =  100 * 31,543,391 = 3,154,339,100  reward will provide for user
                                            # the reward parameter for the last window is multiplied by the time spent in window 0 + window 1, which is the ISSUE
                    totalReward = window.reward * timeSinceReward;
                }
                                    # go out from for
                break;
            }
            unchecked { i++; } # first iteration end just gone to next
        }
    }

4.  Test
Result

[1747110911 - May 13 2026] Bob reward count before stake: 0.0 [1778654302 - May 13 2026] Bob reward count: 533827090.0


Create a new file that includes a part of NeoTokyoStaker and its logic + a test to check the calculation is incorrect
```javascript
"use strict";

// Imports.
import { ethers } from "hardhat";
import { expect, should } from "chai";
import {
  CLASS_TO_ID,
  ASSETS,
  DEDUCE_IDENTITY_CREDIT_YIELDS,
  IDENTITY_CREDIT_POINTS,
  VAULT_MULTIPLIERS,
  VAULT_CREDIT_MULTIPLIER_IDS,
  TIMELOCK_OPTION_IDS,
} from "./utils/constants";
import { prepareContracts } from "./utils/setup";
should();

/*
    Test the updated BYTES token and the staker for correct behavior. Describe the
    contract testing suite, retrieve testing wallets, and create contract
    factories from the artifacts we are testing.
*/
describe("Testing BYTES 2.0 & Neo Tokyo Staker", function () {
  // Track the current time and snapshot ID for reverting between tests.
  let currentTime, snapshotId;

  // Prepare several testing callers.
  let owner, alice, bob, nonCitizen, whale, treasury;

  // Neo Tokyo S1 contract instances.
  let beckLoot, NTItems, NTLandDeploy, vaultBox, NTCitizenDeploy, NTOldBytes;

  // Neo Tokyo S2 contract instances.
  let NTOuterCitizenDeploy, NTOuterIdentity, NTS2Items, NTS2LandDeploy;

  // Various mock testing contracts.
  let CitizenMint, VaultMint, IdentityMint, LPToken;

  // New Neo Tokyo BYTES 2.0 and staker contract instances.
  let NTBytes2_0, NTStaking;

  // Prepare the Neo Tokyo ecosystem before each test.
  before(async () => {
    const signers = await ethers.getSigners();
    const addresses = await Promise.all(
      signers.map(async (signer) => signer.getAddress())
    );

    // Prepare testing callers.
    owner = {
      provider: signers[0].provider,
      signer: signers[0],
      address: addresses[0],
    };
    alice = {
      provider: signers[1].provider,
      signer: signers[1],
      address: addresses[1],
    };
    bob = {
      provider: signers[2].provider,
      signer: signers[2],
      address: addresses[2],
    };
    nonCitizen = {
      provider: signers[3].provider,
      signer: signers[3],
      address: addresses[3],
    };
    whale = {
      provider: signers[4].provider,
      signer: signers[4],
      address: addresses[4],
    };
    treasury = {
      provider: signers[5].provider,
      signer: signers[5],
      address: addresses[5],
    };

    // Prepare all of the Neo Tokyo S1, S2, and new contracts for testing.
    [
      beckLoot,
      NTItems,
      NTLandDeploy,
      vaultBox,
      NTCitizenDeploy,
      NTOldBytes,
      VaultMint,
      IdentityMint,
      LPToken,
      CitizenMint,
      NTOuterCitizenDeploy,
      NTOuterIdentity,
      NTS2Items,
      NTS2LandDeploy,
      NTBytes2_0,
      NTStaking,
    ] = await prepareContracts(treasury.address);
  });

  // Accelerate tests by taking snapshot of the current block before each test.
  beforeEach(async function () {
    currentTime = (await ethers.provider.getBlock()).timestamp;
    snapshotId = await network.provider.send("evm_snapshot");
  });

  // Revert to the snapshot block after each test.
  afterEach(async function () {
    await network.provider.send("evm_revert", [snapshotId]);
  });

  // Prepare to test staker functionality.
  describe("with example configuration", function () {
    // The IDs of Bob's S2 Citizens.
    let s2One;

    // Mint tokens to the testing callers before each test.
    before(async () => {
      await NTLandDeploy.setBytesAddress(NTBytes2_0.address);
      await NTCitizenDeploy.setBytesAddress(NTBytes2_0.address);
      await NTOuterCitizenDeploy.setBytesAddress(NTBytes2_0.address);
      await NTS2Items.setBytesAddress(NTBytes2_0.address);
      await NTS2LandDeploy.setBytesAddress(NTBytes2_0.address);

      async function createS2Citizen(_citizenHolder) {
        let identityIdS2, itemCacheIdS2, landDeedIdS2;
        identityIdS2 = (await NTOuterIdentity.totalSupply()).add(
          ethers.BigNumber.from("1")
        );

        /*
                    Use administrative powers to claim a new S2 Identity for the desired 
                    `_citizenHolder`.
                */
        await NTOuterIdentity.connect(owner.signer).ownerClaim(identityIdS2);
        await NTOuterIdentity.connect(owner.signer).transferFrom(
          owner.address,
          _citizenHolder.address,
          identityIdS2
        );

        // Claim a new S2 Item for the desired holder.
        itemCacheIdS2 = (await NTS2Items.totalSupply()).add(
          ethers.BigNumber.from("1")
        );
        await NTS2Items.connect(owner.signer).emergencyClaim(
          identityIdS2,
          itemCacheIdS2
        );
        await NTS2Items.connect(owner.signer).transferFrom(
          owner.address,
          _citizenHolder.address,
          itemCacheIdS2
        );

        // Claim a new S2 Land Deed for the desired holder.
        landDeedIdS2 = (await NTS2LandDeploy.totalSupply()).add(
          ethers.BigNumber.from("1")
        );
        await NTS2LandDeploy.connect(owner.signer).emergencyClaim(
          identityIdS2,
          landDeedIdS2
        );
        await NTS2LandDeploy.connect(owner.signer).transferFrom(
          owner.address,
          _citizenHolder.address,
          landDeedIdS2
        );

        // Approve the transfer of the holder's components.
        await NTOuterIdentity.connect(_citizenHolder.signer).approve(
          NTOuterCitizenDeploy.address,
          identityIdS2
        );
        await NTS2Items.connect(_citizenHolder.signer).approve(
          NTOuterCitizenDeploy.address,
          itemCacheIdS2
        );
        await NTS2LandDeploy.connect(_citizenHolder.signer).approve(
          NTOuterCitizenDeploy.address,
          landDeedIdS2
        );

        // Assemble the holder's S2 Citizen.
        await NTOuterCitizenDeploy.connect(_citizenHolder.signer).createCitizen(
          identityIdS2,
          itemCacheIdS2,
          landDeedIdS2,
          false,
          ""
        );

        // Return the ID of the new S2 Citizen.
        return NTOuterCitizenDeploy.totalSupply();
      }

      // Create two S2 Citizens for Bob.
      s2One = await createS2Citizen(bob);

      // Have Bob approve the staker to transfer his S2 Citizens.
      await NTOuterCitizenDeploy.connect(bob.signer).approve(
        NTStaking.address,
        s2One
      );

      await NTStaking.connect(owner.signer).configureTimelockOptions(
        ASSETS.S2_CITIZEN.id,
        [...Array(ASSETS.S2_CITIZEN.timelockOptions.length).keys()],
        ASSETS.S2_CITIZEN.timelockOptions
      );

      // Configure each of the stakeable assets with daily reward rates.
      await NTStaking.connect(owner.signer).configurePools([
        {
          assetType: ASSETS.S2_CITIZEN.id,
          daoTax: 0,
          rewardWindows: [
            {
              startTime: 0,
              reward: ethers.utils.parseEther("10"),
            },
          ],
        },
      ]);
    });

    it("ISSUE TEST", async function () {
      // General timePoint's
      // Window 0 startTime = 0
      // Window 1 startTime =  1776227711 = Apr 15 2026
      // Bob last stake time = 1747110911 = May 13 2025
      // Bob get reward time = 1778654302 = May 13 2026

      ethers.provider.send("evm_setNextBlockTimestamp", [1747110911]);
      ethers.provider.send("evm_mine");

      // Stake Bob's S2 Citizens
      let rewards = await NTStaking.getPoolReward(ASSETS.S2_CITIZEN.id, bob.address)
      console.log(`[1747110911 - May 13 2026] Bob reward count before stake: ${ethers.utils.formatEther(rewards[0])}`)

      await NTStaking.connect(bob.signer).stake(
        ASSETS.S2_CITIZEN.id,
        TIMELOCK_OPTION_IDS["30"],
        s2One,
        0,
        0
      );

      await NTStaking.connect(owner.signer).configurePools([
        {
          assetType: ASSETS.S2_CITIZEN.id,
          daoTax: 0,
          rewardWindows: [
            {
              startTime: 0,
              reward: ethers.utils.parseEther("10"),
            },
            {
              startTime: 1776227711,
              reward: ethers.utils.parseEther("100"),
            },
          ],
        },
      ]);

      ethers.provider.send("evm_setNextBlockTimestamp", [1778654302]);
      ethers.provider.send("evm_mine");

      rewards = await NTStaking.getPoolReward(ASSETS.S2_CITIZEN.id, bob.address)
      console.log(`[1778654302 - May 13 2026] Bob reward count: ${ethers.utils.formatEther(rewards[0])}`)
    });
  });
});

Tools Used

Recommended Mitigation Steps

c4-judge commented 1 year ago

hansfriese marked the issue as satisfactory

c4-judge commented 1 year ago

hansfriese marked the issue as duplicate of #423