Venus Prime - blutorque's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L331-L359 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L200-L230

Vulnerability details

Impact

Prime#issue() call allows owner to issue prime tokens to a set of users, its takes isIrrevocable as a paramter which implies to mint revocable/Irrevocable tokens, the call also increases the count of totalRevocable and totalIrrevocable in an internal _mint call. If the scores of these accounts update via updateScores(), the txn will likely to be revert with underflow because the pendingScoreUpdates is not updated for the newly issued accounts above.

Prime.sol

    function _startScoreUpdateRound() internal {
        nextScoreUpdateRoundId++;
        totalScoreUpdatesRequired = totalIrrevocable + totalRevocable;
        pendingScoreUpdates = totalScoreUpdatesRequired;
    }

Consider, first, the owner makes updateAlpha call, it update value of alpha, and internally call the _startScoreUpdateRound() which update pendingScoreUpdates for the total number of accounts exist out there (pendingScoreUpdates perfectly sync here). Secondly, the issue() call happens after, which changes totalRevocable + totalIrrevocable count. Now the pendingScoreUpdates got out of sync with number of accounts there.

So if we try updateScores for all users, including accounts that are not accounted in pendingScoreUpdates, it will revert bc of the underflow here,

NOTE: The user can still update his score via accrueInterestAndUpdateScore for single market at a time. The purpose of report showing here, the updateScores() not working as it suppose to. And it will not be fixed, until alpha, market multipliers and new markets added.

Proof of Concept

Create a new file tests/hardhat/Prime/testPOC.ts, add the following script. And run npx hardhat test --grep "POC" on terminal.

import { FakeContract, MockContract, smock } from "@defi-wonderland/smock";
import { impersonateAccount, loadFixture, mine } from "@nomicfoundation/hardhat-network-helpers";
import { PANIC_CODES } from "@nomicfoundation/hardhat-chai-matchers/panic"; 
import chai from "chai";
import { BigNumber, Signer } from "ethers";
import { ethers } from "hardhat";

import { convertToUnit } from "../../../helpers/utils";
import {
  BEP20Harness,
  ComptrollerLens,
  ComptrollerLens__factory,
  ComptrollerMock,
  ComptrollerMock__factory,
  IAccessControlManager,
  IProtocolShareReserve,
  InterestRateModelHarness,
  PrimeLiquidityProvider,
  PrimeLiquidityProvider__factory,
  PrimeScenario,
  ResilientOracleInterface,
  VBep20Harness,
  XVS,
  XVSStore,
  XVSVault,
  XVSVaultScenario,
} from "../../../typechain";

const { expect } = chai;
chai.use(smock.matchers);

export const bigNumber18 = BigNumber.from("1000000000000000000"); // 1e18
export const bigNumber16 = BigNumber.from("10000000000000000"); // 1e16

type SetupProtocolFixture = {
  oracle: FakeContract<ResilientOracleInterface>;
  accessControl: FakeContract<IAccessControlManager>;
  comptrollerLens: MockContract<ComptrollerLens>;
  comptroller: MockContract<ComptrollerMock>;
  usdt: BEP20Harness;
  vusdt: VBep20Harness;
  eth: BEP20Harness;
  veth: VBep20Harness;
  xvsVault: XVSVaultScenario;
  xvs: XVS;
  xvsStore: XVSStore;
  prime: PrimeScenario;
  protocolShareReserve: FakeContract<IProtocolShareReserve>;
  primeLiquidityProvider: PrimeLiquidityProvider;
};

async function deployProtocol(): Promise<SetupProtocolFixture> {
  const [wallet, user1, user2, user3] = await ethers.getSigners();

  const oracle = await smock.fake<ResilientOracleInterface>("ResilientOracleInterface");
  const protocolShareReserve = await smock.fake<IProtocolShareReserve>("IProtocolShareReserve");
  const accessControl = await smock.fake<IAccessControlManager>("AccessControlManager");
  accessControl.isAllowedToCall.returns(true);
  const ComptrollerLensFactory = await smock.mock<ComptrollerLens__factory>("ComptrollerLens");
  const ComptrollerFactory = await smock.mock<ComptrollerMock__factory>("ComptrollerMock");
  const comptroller = await ComptrollerFactory.deploy();
  const comptrollerLens = await ComptrollerLensFactory.deploy();
  await comptroller._setAccessControl(accessControl.address);
  await comptroller._setComptrollerLens(comptrollerLens.address);
  await comptroller._setPriceOracle(oracle.address);
  await comptroller._setLiquidationIncentive(convertToUnit("1", 18));
  await protocolShareReserve.MAX_PERCENT.returns("100");

  const tokenFactory = await ethers.getContractFactory("BEP20Harness");
  const usdt = (await tokenFactory.deploy(
    bigNumber18.mul(100000000),
    "usdt",
    BigNumber.from(18),
    "BEP20 usdt",
  )) as BEP20Harness;

  const eth = (await tokenFactory.deploy(
    bigNumber18.mul(100000000),
    "eth",
    BigNumber.from(18),
    "BEP20 eth",
  )) as BEP20Harness;

  const wbnb = (await tokenFactory.deploy(
    bigNumber18.mul(100000000),
    "wbnb",
    BigNumber.from(18),
    "BEP20 wbnb",
  )) as BEP20Harness;

  const interestRateModelHarnessFactory = await ethers.getContractFactory("InterestRateModelHarness");
  const InterestRateModelHarness = (await interestRateModelHarnessFactory.deploy(
    BigNumber.from(18).mul(5),
  )) as InterestRateModelHarness;

  const vTokenFactory = await ethers.getContractFactory("VBep20Harness");
  const vusdt = (await vTokenFactory.deploy(
    usdt.address,
    comptroller.address,
    InterestRateModelHarness.address,
    bigNumber18,
    "VToken usdt",
    "vusdt",
    BigNumber.from(18),
    wallet.address,
  )) as VBep20Harness;
  const veth = (await vTokenFactory.deploy(
    eth.address,
    comptroller.address,
    InterestRateModelHarness.address,
    bigNumber18,
    "VToken eth",
    "veth",
    BigNumber.from(18),
    wallet.address,
  )) as VBep20Harness;
  const vbnb = (await vTokenFactory.deploy(
    wbnb.address,
    comptroller.address,
    InterestRateModelHarness.address,
    bigNumber18,
    "VToken bnb",
    "vbnb",
    BigNumber.from(18),
    wallet.address,
  )) as VBep20Harness;

  //0.2 reserve factor
  await veth._setReserveFactor(bigNumber16.mul(20));
  await vusdt._setReserveFactor(bigNumber16.mul(20));

  oracle.getUnderlyingPrice.returns((vToken: string) => {
    if (vToken == vusdt.address) {
      return convertToUnit(1, 18);
    } else if (vToken == veth.address) {
      return convertToUnit(1200, 18);
    }
  });

  oracle.getPrice.returns((token: string) => {
    if (token == xvs.address) {
      return convertToUnit(3, 18);
    }
  });

  const half = convertToUnit("0.5", 18);
  await comptroller._supportMarket(vusdt.address);
  await comptroller._setCollateralFactor(vusdt.address, half);
  await comptroller._supportMarket(veth.address);
  await comptroller._setCollateralFactor(veth.address, half);

  await eth.transfer(user1.address, bigNumber18.mul(100));
  await usdt.transfer(user2.address, bigNumber18.mul(10000));

  await comptroller._setMarketSupplyCaps([vusdt.address, veth.address], [bigNumber18.mul(10000), bigNumber18.mul(100)]);

  await comptroller._setMarketBorrowCaps([vusdt.address, veth.address], [bigNumber18.mul(10000), bigNumber18.mul(100)]);

  const xvsFactory = await ethers.getContractFactory("XVS");
  const xvs: XVS = (await xvsFactory.deploy(wallet.address)) as XVS;

  const xvsStoreFactory = await ethers.getContractFactory("XVSStore");
  const xvsStore: XVSStore = (await xvsStoreFactory.deploy()) as XVSStore;

  const xvsVaultFactory = await ethers.getContractFactory("XVSVaultScenario");
  const xvsVault: XVSVaultScenario = (await xvsVaultFactory.deploy()) as XVSVaultScenario;

  await xvsStore.setNewOwner(xvsVault.address);
  await xvsVault.setXvsStore(xvs.address, xvsStore.address);
  await xvsVault.setAccessControl(accessControl.address);

  await xvs.transfer(xvsStore.address, bigNumber18.mul(1000));
  await xvs.transfer(user1.address, bigNumber18.mul(1000000));
  await xvs.transfer(user2.address, bigNumber18.mul(1000000));
  await xvs.transfer(user3.address, bigNumber18.mul(1000000));

  await xvsStore.setRewardToken(xvs.address, true);

  const lockPeriod = 300;
  const allocPoint = 100;
  const poolId = 0;
  const rewardPerBlock = bigNumber18.mul(1);
  await xvsVault.add(xvs.address, allocPoint, xvs.address, rewardPerBlock, lockPeriod);

  const primeLiquidityProviderFactory = await ethers.getContractFactory("PrimeLiquidityProvider");
  const primeLiquidityProvider = await upgrades.deployProxy(
    primeLiquidityProviderFactory,
    [accessControl.address, [xvs.address, usdt.address, eth.address], [10, 10, 10]],
    {},
  );

  const primeFactory = await ethers.getContractFactory("PrimeScenario");
  const prime: PrimeScenario = await upgrades.deployProxy(
    primeFactory,
    [
      xvsVault.address,
      xvs.address,
      0,
      1,
      2,
      accessControl.address,
      protocolShareReserve.address,
      primeLiquidityProvider.address,
      comptroller.address,
      oracle.address,
      10,
    ],
    {
      constructorArgs: [wbnb.address, vbnb.address, 10512000],
      unsafeAllow: "constructor",
    },
  );

  await xvsVault.setPrimeToken(prime.address, xvs.address, poolId);

  await prime.setLimit(1000, 1000);

  await prime.addMarket(vusdt.address, bigNumber18.mul("1"), bigNumber18.mul("1"));

  await prime.addMarket(veth.address, bigNumber18.mul("1"), bigNumber18.mul("1"));

  await comptroller._setPrimeToken(prime.address);

  await prime.togglePause();

  return {
    oracle,
    comptroller,
    comptrollerLens,
    accessControl,
    usdt,
    vusdt,
    eth,
    veth,
    xvsVault,
    xvs,
    xvsStore,
    prime,
    protocolShareReserve,
    primeLiquidityProvider,
  };
}

describe("setup", () => {
  let deployer: Signer;
  let user1: Signer;
  let user2: Signer;
  let user3: Signer;

  let prime: PrimeScenario;
  let xvsVault: XVSVault;
  let xvs: XVS;

  beforeEach(async () => {
    [deployer, user1, user2, user3] = await ethers.getSigners();
    ({ prime, xvsVault, xvs } = await loadFixture(deployProtocol));
  });

  it.only("POC", async () => { 
    // deposit to xvsVault for user1 and user2
    await xvs.connect(user1).approve(xvsVault.address, bigNumber18.mul(10000)); 
    await xvsVault.connect(user1).deposit(xvs.address, 0, bigNumber18.mul(10000)); 

    await xvs.connect(user2).approve(xvsVault.address, bigNumber18.mul(5000)); 
    await xvsVault.connect(user2).deposit(xvs.address, 0, bigNumber18.mul(5000)); 
    // configure the timestamp to 90 days
    await mine(90 * 24 * 60 * 60);

    // user become eligible to claimInterest on their deposit after they claim prime token
    await prime.connect(user1).claim();
    await prime.connect(user2).claim();
    
    // update the alpha to 0.8
    await prime.updateAlpha(4,5); 

    expect(await prime.pendingScoreUpdates()).to.be.equal(2);	  
    // mint an irrevocable token without changing the pendingScoreUpdates for user3 also
    await prime.issue(true, [user3.getAddress()]);

    // the txn revert due to underflow
    await expect(prime.updateScores([user1.getAddress(), user2.getAddress(), user3.getAddress()]))
      .to.be.revertedWithPanic(PANIC_CODES.ARITHMETIC_UNDER_OR_OVERFLOW);	
  }) 
});

Tools Used

Manual review

Recommended Mitigation Steps

    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]);
                }

                unchecked {
                    i++;
                }
            }
        } else {
            for (uint256 i = 0; i < users.length; ) {
                _mint(false, users[i]);
                _initializeMarkets(users[i]);
                delete stakedAt[users[i]];

                unchecked {
                    i++;
                }
            }
        }
+       totalScoreUpdatesRequired = totalIrrevocable + totalRevocable;
+       pendingScoreUpdates = totalScoreUpdatesRequired;
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Whenever, any factors like alpha and market multiplier updates via their corresponding set function, the internal call _startScoreUpdateRound increments the nextScoreUpdateRoundId signaling user to update their score for next round.

Prime#updateScores() allows changing multiple users score for the new nextScoreUpdateRoundId, it call _updateScore for each user in the users[] array for every single market.

And If it try to execute for the same user twice for the same nextScoreUpdateRoundId, this check will triggered and skip the further operations for that user. However, it not working as it supposed to, bc to execute loop further i need to be incremented. And if we are skipping "further operation" like I said above, we are also skipping the incrementation of i here. This will cause loop to execute for the same value of i until the block gas limit reached.

Proof of Concept

Consider we update the multipliers for vUSDT market,

• updateScores() is being called for the X users, includes duplicate too(say index 6 and 9).
• the for-loop executed for 6th index user, marking isScoreUpdated[nextScoreUpdateRoundId][user] = true for the nextScoreUpdateRoundId
• it tries for the 9th index later, figures that isScoreUpdated[nextScoreUpdateRoundId][user] is already set true for the current roundId, and skipped with continue,
• however, unchecked { ++i; } also got skipped, making for-loop to loop forever
• finally, the block gas limit reached and the txn reverts,

Tools Used

Manual review

Recommended Mitigation Steps

    function updateScores(address[] memory users) external {
        if (pendingScoreUpdates == 0) revert NoScoreUpdatesRequired();
        if (nextScoreUpdateRoundId == 0) revert NoScoreUpdatesRequired();

        for (uint256 i = 0; i < users.length; ++i) {
            address user = users[i];

            if (!tokens[user].exists) revert UserHasNoPrimeToken();
            if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue; 

            address[] storage _allMarkets = allMarkets;
            for (uint256 j = 0; j < _allMarkets.length; ) {
                address market = _allMarkets[j];
                _executeBoost(user, market);
                _updateScore(user, market);

                unchecked {
                    j++;
                }
            }

            pendingScoreUpdates--;
            isScoreUpdated[nextScoreUpdateRoundId][user] = true;

            emit UserScoreUpdated(user);
        }
    }

Assessed type

Loop

[Low-01]: Adversary can prevent the issuing of prime token, by frontrunning with claim call

The issue call takes users[] as input, to which the revocable/irrevocable token to mint. If the users arr include the address which is already eligible for revocable tokens. Then, any issue call for revocable token can be prevented with frontrunning claim call by the adversary.

   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 claim internally call _mint function, which changes tokens[user].exists = true, it also make sure if the token minted for same user again, it revert in first line.

    function _mint(bool isIrrevocable, address user) internal {
        if (tokens[user].exists) revert IneligibleToClaim(); 
        
        tokens[user].exists = true; 
        // ...SNIP...
    }

When the issue iterate over array of users for the adversary address, the txn will revert with IneligibleToClaim err, bc the token for adversary already exists.

Recommendation File: Prime.sol

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

        if (isIrrevocable) {
        // ...SNIP...
        } else {
            for (uint256 i = 0; i < users.length; ++i) {
                if (tokens[users[i]].exists) continue; 

                _mint(false, users[i]);
                _initializeMarkets(users[i]);
                delete stakedAt[users[i]];
            }
        }
    }

[Low-02]: Scores#calculateScore() doing unecessary computation for alpha=0 and alpha=1

The calculate score uses the formula xvs^𝝰 * capital^(1-𝝰), where the alpha lies b/w (0,1). Its also possible for the value of alpha to be either 0 or 1. However, if it so, the calculateScore performing unnecesary logarithmic and exponentials calculation, increasing the complexity and adding more attack surfaces, which is definitely not needed.

As looking at the above formula, we can say for 𝝰=0, the score is corresponding to capital and for 𝝰=1, the score eq staked xvs

This value can be directly returns.

Recommendation File: Scores.sol

    function calculateScore(
        uint256 xvs,
        uint256 capital,
        uint256 alphaNumerator,
        uint256 alphaDenominator
    ) internal pure returns (uint256) {
   // ...SNIP...
        if (xvs == capital) return xvs;

+      if(alphaNumerator == 0) return capital;
+      if(alphaNumerator == alphaDenominator) return xvs; 

        bool xvsLessThanCapital = xvs < capital;

        // (xvs / capital) or (capital / xvs), always in range (0, 1)
        int256 ratio = xvsLessThanCapital ? FixedMath.toFixed(xvs, capital) : FixedMath.toFixed(capital, xvs);

        // e ^ ( ln(ratio) * 𝝰 )
        int256 exponentiation = FixedMath.exp(
            (FixedMath.ln(ratio) * alphaNumerator.toInt256()) / alphaDenominator.toInt256()
        );

        if (xvsLessThanCapital) {
            // capital * e ^ (𝝰 * ln(xvs / capital))
            return FixedMath.uintMul(capital, exponentiation);
        }

        // capital / e ^ (𝝰 * ln(capital / xvs))
        return FixedMath.uintDiv(capital, exponentiation);
    }

[Low-03]: _claimInterest set the user rewardIndex to non-zero, when its already set to 0 during _burn call

The burn is restricted call, can be called to burn prime token. The function internally calls _burn which first accrue rewards for the user and then set the user score and rewardIndex to zero.

    function _interestAccrued(address vToken, address user) internal view returns (uint256) {
        uint256 index = markets[vToken].rewardIndex - interests[vToken][user].rewardIndex;
        uint256 score = interests[vToken][user].score;

        return (index * score) / EXP_SCALE;
    }

Aftwards the burn call, user still can claim her collected rewards via _claimInterest function. However, this call changes the user rewardIndex from 0 to a non-zero market rewardIndex, which could open unexpected attack paths.

Recommendation File: Prime.sol

    function _claimInterest(address vToken, address user) internal returns (uint256) {
        uint256 amount = getInterestAccrued(vToken, user);
        amount += interests[vToken][user].accrued;

        if(tokens[user].exists) interests[vToken][user].rewardIndex = markets[vToken].rewardIndex;
        interests[vToken][user].accrued = 0;
        // ...SNIP...
    }

[Low-04]: Revocable token issued by owner can be immediately burn, reducing the pendingScoreUpdates by 1

The issue function directly issue prime tokens to users. It do not check the user eligibility, when minting the revocable tokens. If the xvsUpdated function which has a public visibility is being called for that user, it eventually _burn the revocable tokens of that users.

    function xvsUpdated(address user) external {
        uint256 totalStaked = _xvsBalanceOfUser(user);
        bool isAccountEligible = isEligible(totalStaked);

        if (tokens[user].exists && !isAccountEligible) {
            if (tokens[user].isIrrevocable) {
                _accrueInterestAndUpdateScore(user);
            } else {
                _burn(user);  // user hold revocable token, but not eligible
            }
        } else if (!isAccountEligible && !tokens[user].exists && stakedAt[user] > 0) {
        // ...SNIP...
    }

However, the problem is _burn internally calls _updateRoundAfterTokenBurned, which reduces the pendingScoreUpdates count by 1. We knew already that, the pendingScoreUpdates never actually been increment or updated when the revocable token issued.

    function _updateRoundAfterTokenBurned(address user) internal {
        if (totalScoreUpdatesRequired > 0) totalScoreUpdatesRequired--;

        if (pendingScoreUpdates > 0 && !isScoreUpdated[nextScoreUpdateRoundId][user]) {
            pendingScoreUpdates--;
        }
    }

This could lead to an unexpected behaviour at the places where it suppose to be higher than the current value.