search

code-423n4 / 2023-09-venus-findings

4 stars 4 forks source link

Prime#issue() could DoS any future call that update the user score via `updateScores()`. #217

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

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

c4-pre-sort commented 1 year ago

0xRobocop marked the issue as duplicate of #555

c4-judge commented 1 year ago

fatherGoose1 marked the issue as satisfactory