Venus Prime - merlin's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L339-L342

Vulnerability details

Bug Description

Here's the step-by-step description of this issue:

1. A user deposits(XVSVault.deposit) 10,000 XVS tokens for a certain period (e.g., 90 days).
2. ACM issues(Prime.issue) an irrevocable prime token to the user.
3. The user withdraws(XVSVault.requestWithdrawal) 9,500 XVS tokens.
4. ACM burns(Prime.burn) the irrevocable prime token from the user.
5. With a remaining balance of 500 XVS tokens, the user immediately claims(Prime.claim) a revocable prime token , without waiting for the full 90 days.

Proof-of-Concept

Include and execute a test in the tests/hardhat/Prime/Prime.ts file:

it("mint recovable prime token to user faster than 90 days", async () => {
      const user = user1;

      await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(10000));
      await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(10000));
      await mine(89 * 24 * 60 * 60); //pass 89 days

      await expect(prime.connect(user).claim()).to.be.revertedWithCustomError(prime, "WaitMoreTime");

      await prime.issue(true, [user1.getAddress()]);
      expect(await prime.totalIrrevocable()).to.be.equal(1);

      let token = await prime.tokens(user.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(true);

      // user withdraw almost all XVS tokens
      await xvsVault.connect(user).requestWithdrawal(xvs.address, 0, bigNumber18.mul(9500));

      // ACM burn irrecovable prime token for user
      await prime.burn(user.getAddress());
      token = await prime.tokens(user.getAddress());
      expect(token.exists).to.be.equal(false);
      expect(token.isIrrevocable).to.be.equal(false);

      // user claim recovable prime token after 2 days instead of 90
      await mine(2 * 24 * 60 * 60); //pass 2 days
      await prime.connect(user).claim();

      token = await prime.tokens(user.getAddress());
      expect(token.exists).to.be.equal(true);
      expect(token.isIrrevocable).to.be.equal(false);
    });

Impact

After ACM burns the irrevocable prime token from the user, the user can instantly claim revocable token, regardless of having less than 1,000 XVS tokens and without the requirement of staking for the full 90 days once more. This issue arises only when irrevocable prime tokens are directly given to the user. When ACM upgrades revocable to irrevocable prime tokens, the user has already claimed the revocable prime token, and their stakedAt value was set to 0.

Tools Used

Manual review

Recommended Mitigation Steps

Consider including the following line within the issue function:

function issue(bool isIrrevocable, address[] calldata users) external {
        _checkAccessAllowed("issue(bool,address[])");

        if (isIrrevocable) {
            for (uint256 i = 0; i < users.length; ) {
                Token storage userToken = tokens[users[i]];
                //
                if (userToken.exists && !userToken.isIrrevocable) {
                    _upgrade(users[i]);
                } else {
                    _mint(true, users[i]);
                    _initializeMarkets(users[i]);
+			delete stakedAt[users[i]];
                }

                unchecked {
                    i++;
                }
            }
        } else {
            // code
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L255

Vulnerability details

Bug Description

As stated in the Arbitrum Docs, the block.number function returns the most recently synchronized L1 block number. This synchronization between the block number in the Sequencer and the actual L1 block number occurs once every minute.

Proof-of-Concept

L2 block.number: updated to sync with L1 block.number ~ every minute (source): L1 block.number 1000 1001 1002 1003 1004 1005 L2 block.number 1000 1000 1000 1000 1004 1004

The PrimeLiquidityProvider.accrueTokens function will utilize an L1 block.number for the Arbitrum chain.

Impact

The calculation of tokenAmountAccrued will be done using the Ethereum L1 block.number, not the L2 Arbitrum block.number. Additionally, the actual L1 block number occurs once every minute.

Tools Used

Manual review

Recommended Mitigation Steps

Consider utilizing the L2 block.number for computing tokenAmountAccrued:

+uint256 constant public ARBITRUM_CHAIN_ID = 42161;
    ArbSys constant public arbSys = +ArbSys(address(100));
	
function getBlockNumber() public view virtual returns (uint256) {
+if(block.chainid == ARBITRUM_CHAIN_ID)
+{
+	return arbSys.arbBlockNumber();
+}        
return block.number;
    }

Assessed type

Timing