Venus Prime - ThreeSigma's results

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L207

Vulnerability details

Impact

If a user no longer exists before the call to updateScores() has been made, the whole transaction will revert and will not allow for the other user's scores to be updated, incurring in extra gas costs by having to call this function again with a new input. This is especially relevant because, according to the "Update cap multipliers and alpha" section of the README, the call to updateScores() is done by a script developed by Venus. They intend to,for example, after adding a new market, update all the scores using updateScores(), but users could leave the market on purpose, possibly making big batches of user score updates revert, leading to DoS and massive gas costs.

Proof of Concept

The highlighted line below shows how the transaction reverts if a user has exited:

function updateScores(address[] memory users) external {
    ...
    if (!tokens[user].exists) revert UserHasNoPrimeToken();
    ...
}

Insert the following test in Prime.ts:describe("update score",...):

it.only("POC update scores fails due to user exiting a market", async () => {
  await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
  await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));
  await prime.issue(false, [user3.getAddress()]);
  await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));

  let interest = await prime.interests(vbnb.address, user3.getAddress());
  expect(interest.accrued).to.be.equal(0);
  expect(interest.score).to.be.equal(0);
  expect(interest.rewardIndex).to.be.equal(0);

  let market = await prime.markets(vbnb.address);
  expect(market.supplyMultiplier).to.be.equal(bigNumber18.mul(1));
  expect(market.borrowMultiplier).to.be.equal(bigNumber18.mul(1));
  expect(market.rewardIndex).to.be.equal(0);
  expect(market.sumOfMembersScore).to.be.equal(0);

  let nextScoreUpdateRoundId = await prime.nextScoreUpdateRoundId();
  let totalScoreUpdatesRequired = await prime.totalScoreUpdatesRequired();
  let pendingScoreUpdates = await prime.pendingScoreUpdates();
  let isScoreUpdated = await prime.isScoreUpdated(nextScoreUpdateRoundId, user3.getAddress());
  expect(nextScoreUpdateRoundId).to.be.equal(3);
  expect(totalScoreUpdatesRequired).to.be.equal(2);
  expect(pendingScoreUpdates).to.be.equal(2);
  expect(isScoreUpdated).to.be.equal(false);

  await xvsVault.connect(user3).requestWithdrawal(xvs.address, 0, bigNumber18.mul(2000));

  await expect(prime.updateScores([user1.getAddress(), user3.getAddress()])).to.be.revertedWithCustomError(prime, "UserHasNoPrimeToken");
});

Tools Used

Vscode, Hardhat

Recommended Mitigation Steps

Skip the user that does not exist instead of reverting.

Assessed type

Loop

Lines of code

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

Vulnerability details

Impact

According to the documentation, one of the goals of updateScores() is, after a market is added, update the score of all users. In this case, as the function is expected to be called with a large array, it would incur in DoS and significant gas costs if the transaction were to revert. This is the case if new tokens are issued and updateScores() is called afterwards, as neither pendingScoreUpdates or nextScoreUpdateRoundId are increased.

Proof of Concept

Function updateScores in Prime decrements pendingScoreUpdates each time a score is updated. Thus, if a new token is issued, but this variable is not increased, it won't be possible to update all users, if one of the new users calls updateScores() with its address. The vulnerable line is highlighted below, which will underflow:

function updateScores(address[] memory users) external {
    ...
    pendingScoreUpdates--;
    ...
}

Add the following test to Prime.ts:describe("update scores", ...):

it.only("POC update scores fails due to token being issued", async () => {
  await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
  await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));
  await prime.issue(false, [user3.getAddress()]);
  await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));

  // token issued after adding market
  await prime.issue(false, [user2.getAddress()]);

  let interest = await prime.interests(vbnb.address, user3.getAddress());
  expect(interest.accrued).to.be.equal(0);
  expect(interest.score).to.be.equal(0);
  expect(interest.rewardIndex).to.be.equal(0);

  let market = await prime.markets(vbnb.address);
  expect(market.supplyMultiplier).to.be.equal(bigNumber18.mul(1));
  expect(market.borrowMultiplier).to.be.equal(bigNumber18.mul(1));
  expect(market.rewardIndex).to.be.equal(0);

  let nextScoreUpdateRoundId = await prime.nextScoreUpdateRoundId();
  let totalScoreUpdatesRequired = await prime.totalScoreUpdatesRequired();
  let pendingScoreUpdates = await prime.pendingScoreUpdates();
  let isScoreUpdated = await prime.isScoreUpdated(nextScoreUpdateRoundId, user3.getAddress());
  expect(nextScoreUpdateRoundId).to.be.equal(3);
  expect(totalScoreUpdatesRequired).to.be.equal(2);
  expect(pendingScoreUpdates).to.be.equal(2);
  expect(isScoreUpdated).to.be.equal(false);

  // update user2 so user1 and user3 won't have enough pending score updates
  await prime.updateScores([user2.getAddress()]);

  await expect(prime.updateScores([user1.getAddress(), user3.getAddress()])).to.be.revertedWithPanic("0x11");
});

Tools Used

Vscode, Hardhat

Recommended Mitigation Steps

Call the internal function _startScoreUpdateRound() at the end of function issue().

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L208

Vulnerability details

Impact

User that calls updateScores() where an account's score was already updated will enter an endless loop, reverting the transaction and spending all the gas.

Proof of Concept

The error occurs due to doing continue, but the i variable only being incremented at the end of the loop, in a unchecked statement:

function updateScores(address[] memory users) external {
    ...
    for (uint256 i = 0; i < users.length; ) {
        ...
        if (isScoreUpdated[nextScoreUpdateRoundId][user]) continue; // here, it never increments the variable i
        ...
        unchecked {
            i++;
        }
        ...
    }
}

A POC was carried out to confirm the behaviour. Add the following test to Prime.ts:describe("update score", ...):

it.only("POC update scores infinite loop", async () => {
  await xvs.connect(user3).approve(xvsVault.address, bigNumber18.mul(2000));
  await xvsVault.connect(user3).deposit(xvs.address, 0, bigNumber18.mul(2000));
  await prime.issue(false, [user3.getAddress()]);
  await prime.addMarket(vbnb.address, bigNumber18.mul(1), bigNumber18.mul(1));

  let interest = await prime.interests(vbnb.address, user3.getAddress());
  expect(interest.accrued).to.be.equal(0);
  expect(interest.score).to.be.equal(0);
  expect(interest.rewardIndex).to.be.equal(0);

  let market = await prime.markets(vbnb.address);
  expect(market.supplyMultiplier).to.be.equal(bigNumber18.mul(1));
  expect(market.borrowMultiplier).to.be.equal(bigNumber18.mul(1));
  expect(market.rewardIndex).to.be.equal(0);
  expect(market.sumOfMembersScore).to.be.equal(0);

  let nextScoreUpdateRoundId = await prime.nextScoreUpdateRoundId();
  let totalScoreUpdatesRequired = await prime.totalScoreUpdatesRequired();
  let pendingScoreUpdates = await prime.pendingScoreUpdates();
  let isScoreUpdated = await prime.isScoreUpdated(nextScoreUpdateRoundId, user3.getAddress());
  expect(nextScoreUpdateRoundId).to.be.equal(3);
  expect(totalScoreUpdatesRequired).to.be.equal(2);
  expect(pendingScoreUpdates).to.be.equal(2);
  expect(isScoreUpdated).to.be.equal(false);

  await prime.updateScores([user1.getAddress()]);

  await prime.updateScores([user1.getAddress(), user3.getAddress()]);
});

Tools Used

Vscode, Hardhat

Recommended Mitigation Steps

Either increment if the condition to skip the current iteration is verified or increment the i variable normally in the loop statement.

Assessed type

Loop

Low

[L - 1] Prime - Round id is only considered on updateScores(), not in other functions. The function updateScores() checks the variables pendingScoreUpdates and nextScoreUpdateRoundId before allowing for updates to happen, and after updates pendingScoreUpdates and isScoreUpdated. These conditions can be easily circumvented by calling the function accrueInterestAndUpdateScore() that allows for updates to happen without going through any of the previously mentioned conditions.

[L - 2] There is no input validation for the updateMultiplier function In this function, neither the variables supplyMultiplier nor borrowMultiplier are checked to be between reasonable values

Non-Critical Issues

[N - 1] PrimeLiquidityProvider Change function name _ensureZeroAddress -> _ensureValidAddress The function _ensureZeroAddress does not ensure that the address given is a zero address, like the name suggests, it ensures that it is a valid address (i.e. not a zero address), and thus it should be named _ensureValidAddress

[N - 2] PrimeLiquidityProvider, lastAccruedBlock Incorrect comment
The comment "/// @notice The rate at which token is distributed to the Prime contract" does not accurately describe the variable. A more accurate comment could be "The block number of the latest accrual for that token"

[N - 3] Prime - There is no guarantee that when a new round starts, all users have updated their scores. There are several functions (updateAlpha, updateMultipliers and addMarket) that call the function _startScoreUpdateRound , this function updates the values nextScoreUpdateRoundId, totalScoreUpdatesRequired, and pendingScoreUpdates. However, there is no check to ensure that the previous round of updates has ended and therefore there are still pendingScoreUpdates for that specific round.