Prime.sol currently miscalculates the duration users have already staked, which breaks multiple core functions.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L473-L487 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L394-L405

Vulnerability details

Summary

A wrong assumption is currently being made regarding the time taken to mine a block in all chains where the protocol will be deployed this is cause multiple core functions inappropriately equate block per year to seconds per year.

Impact

The impact is significant, greatly affecting different sections of Prime.sol. This report focuses on two main areas:

• The claimTimeRemaining() function would never function proper except if the

• Users consistently encounter the WaitMoreTime() error when attempting to claim their rewards using the claim() function. Even if the STAKING_PERIOD has lapsed, they are unable to claim. For instance, on the Ethereum mainnet, instead of waiting 90 days, users would need to wait nearly 3 years (precisely 1080 days) to claim rewards.

  Note: The waiting duration varies based on the chain where the contract is deployed. The 3-year delay on Ethereum arises because blocks are mined on average every 12 seconds. On the other hand, if we consider the Binance chain as an example, users would need to wait for 9 months (precisely 270 days) due to blocks being mined every 3 seconds.

Proof of Concept

The ReadMe's Additional Context section states:

Blockchains where this code will be deployed: BNB Chain, Ethereum mainnet, Arbitrum, Polygon zkEVM, opBNB.

Part 1

Take a look at Prime.sol#L473-L487

function claimTimeRemaining(address user) external view returns (uint256) {
  if (stakedAt[user] == 0) return STAKING_PERIOD;

  uint256 totalTimeStaked = block.timestamp - stakedAt[user];
  if (totalTimeStaked < STAKING_PERIOD) {
    return STAKING_PERIOD - totalTimeStaked;
  } else {
    return 0;
  }
}

This function determines the remaining seconds until a user's staking period is complete. The central issue arises from using the difference in block.timestamp to gauge the duration a user has staked:

 uint256 totalTimeStaked = block.timestamp - stakedAt[user];

Given the plan to deploy the protocol on diverse chains, such as Ethereum mainnet, Arbitrum, Polygon zkEVM, etc., let's delve into a step-by-step POC using the Ethereum mainnet:

• STAKING_PERIOD is 90 days, equivalent to 7,776,000 seconds.
• On the Ethereum mainnet, blocks take 12 seconds to mine.
• After 90 days, the Ethereum mainnet's block.timestamp difference would be 7,776,000/12, or 648,000 blocks.

Now when a user stakes and then calls the claimTimeRemaining() function after 8,640,000 seconds 100 days the function should rightly return 0 since 8,640,000 > 7,776,000, but this is what happens instead:

• Ethereum Mainnet's timestamp is 100,000 (100,000 arbitrarily selected for POC).
• User stakes, setting stakedAt[user] to 100,000.
• After a duration of 8,640,000 seconds.
• Ethereum Mainnet's timestamp becomes 820,000, calculated from 8,640,000/12 = 720,000 which gets added to 100,000 when we user first staked.
• User invokes claimTimeRemaining().
• The first check, if (stakedAt[user] == 0) return STAKING_PERIOD; will pass, as stakedAt[user] == 100,000.
• uint256 totalTimeStaked = block.timestamp - stakedAt[user]; results in 720,000.
• The if condition: if (totalTimeStaked < STAKING_PERIOD) would evaluate to true, cause we are attempting 720,000 < 7,776,000 and then the returned value would be 7,056,000 which is ~82 days.
• Evidently after more than 100 days since the user staked, they still get informed that they have 82 days left to be eligible to claim rewards.
• This means that on the Ethereum mainnet the claimTimeRemaining() actually evaluates with *12 the value of STAKING_PERIOD, expecting over a 1,000 days, precisely 1080 days.
• Key to note that the overvaluation is dependent on the chain of deployment, if it's on Binance then this would be *3 the value since BSC takes approximately 3 seconds to mine a block.

Part 2

Take a look at Prime.sol#L394-L405

function claim() external {
  if (stakedAt[msg.sender] == 0) revert IneligibleToClaim();
  if (block.timestamp - stakedAt[msg.sender] < STAKING_PERIOD) revert WaitMoreTime();

  stakedAt[msg.sender] = 0;

  _mint(false, msg.sender);
  _initializeMarkets(msg.sender);
}

The above function is used to claim prime token whenever the staking period is completed, i.e after STAKING_PERIOD has passed.

If a user tries claiming their prime tokens after 100 days , the line below would block the attempted claim.

if (block.timestamp - stakedAt[msg.sender] < STAKING_PERIOD) revert WaitMoreTime();

Here, block.timestamp - stakedAt[msg.sender] equates to a comparatively smaller value against STAKING_PERIOD. Specifically, 620,000 versus 7,776,000, making the if statement true, and the function reverts with the WaitMoreTime() error.

Please refer back to the Step-by-Step POC from Part 1to understand more on how we come by these values, i.e 620,000 & 7,776,000

This demonstrates that, for instance, on the Ethereum mainnet, users are unjustly denied from claiming prime tokens, being forced to wait nearly 3 years to obtain what is rightfully theirs.

Hardhat POC

it("stake and mint does not work for Bauchibred", async () => {
  const user = user1;
  // Here we ensure an ineligible user can't claim
  await expect(prime.connect(user).claim()).to.be.revertedWithCustomError(prime, "IneligibleToClaim");
  // User approves and deposits tokens
  await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(10000));
  await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(10000));
  // Here we confirm that the user has staked tokens
  let stake = await prime.stakedAt(user.getAddress());
  expect(stake).be.gt(0);
  // Here we ensure the user can't claim right after staking
  await expect(prime.connect(user).claim()).to.be.revertedWithCustomError(prime, "WaitMoreTime");
  // Here we check the time remaining for the user to claim
  expect(await prime.claimTimeRemaining(user.getAddress())).to.be.equal(7775999);
  // Fast forward 100 days (90 * 24 * 60 * 60 / 12 seconds)
  await mine((100 * 24 * 60 * 60) / 12);
  //  Here we confirm that claimTimeRemaining() informs users that they still have to wait for ~82 days
  expect(await prime.claimTimeRemaining(user.getAddress())).to.be.equal(7055999);

  // Here we confirm that even after 100 days, users still get the "WaitMoreTime" error if they attempt to claim.
  await expect(prime.connect(user).claim()).to.be.revertedWithCustomError(prime, "WaitMoreTime");
  // Fast forward 979 more days, in an addition to the  previously mined 100 = 1079 days
  await mine((979 * 24 * 60 * 60) / 12);
  // Here we confirm that users  still can't claim prime tokens after a 1079 days
  await expect(prime.connect(user).claim()).to.be.revertedWithCustomError(prime, "WaitMoreTime");
  // Fast forward the last day to get us to 1080 days
  await mine((1 * 24 * 60 * 60) / 12);
  // Finally, users can now claim their tokens.
  // Also we've confirmed that on the Ethereum mainnet, users can only claim tokens after 36 months (1080 days) and not 90 days
  await expect(prime.connect(user).claim()).to.be.not.reverted;
});

Steps to Reproduce POC

• Navigate to Prime.ts
• Add the above test as an instance under the describe("mint and burn", () test cases
• Pass this command to the terminal yarn test --grep "mint and burn"
• The test would pass as shown below

Tool Used

• Manual review
• Hardhat

Recommended Mitigation Steps

Part 1

Since STAKING_PERIOD is measured in seconds, the totalTimeStaked value should be converted to seconds before checking against STAKING_PERIOD. This means multiplying totalTimeStaked by BLOCKS_PER_YEAR:

...
function claimTimeRemaining(address user) external view returns (uint256) {
  if (stakedAt[user] == 0) return STAKING_PERIOD;

-  uint256 totalTimeStaked = block.timestamp - stakedAt[user];
+  uint256 totalTimeStaked = (block.timestamp - stakedAt[user]) * BLOCKS_PER_YEAR;
  if (totalTimeStaked < STAKING_PERIOD) {
    return STAKING_PERIOD - totalTimeStaked;
  } else {
    return 0;
  }
}

Part 2

Similar to the Part 1 fix, the difference between block.timestamp and stakedAt[msg.sender] should be transformed to seconds before comparing it to STAKING_PERIOD. For clarity, this difference can be stored as a separate variable, similar to claimTimeRemaining(), before performing the check:

    function claim() external {
        if (stakedAt[msg.sender] == 0) revert IneligibleToClaim();
-       if (block.timestamp - stakedAt[msg.sender] < STAKING_PERIOD) revert WaitMoreTime();
+       uint256 totalTimeStaked = (block.timestamp - stakedAt[msg.sender]) * BLOCKS_PER_YEAR;
+       if (totalTimeStaked < STAKING_PERIOD) revert WaitMoreTime();
        stakedAt[msg.sender] = 0;

        _mint(false, msg.sender);
        _initializeMarkets(msg.sender);
    }

Assessed type

Timing

[0xRobocop]

Invalid

[c4-pre-sort]

0xRobocop marked the issue as low quality report

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[fatherGoose1]

Confusing block.timestamp with block.number.