Venus Prime - berlin-101's results

Lines of code

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

Vulnerability details

Impact

User with revocable Prime token loses 90 days of Prime rewards when getting upgraded to irrevocable prime token and then burned

Proof of Concept

The protocol can issue (irrevocable and revocable) Prime tokens manually via the issue() function in Prime.sol (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L331).

Here we want to compare 2 scenarios with each other that involve using this functionality. Each scenario is additionally covered with a coded POC (please see below).

I.) User that did not claim is at advantage

1. User has staked the minimum amount of XVS tokens to be eligible for claiming a revocable prime token by calling claim() in Prime.sol (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L397).
2. His stakedAt time is set to the timestamp when the first time xvsUpdated() was called after he staked the minimum XVS amount (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L378)
3. The user DOES NOT call claim() so he does not get a revocable prime token; his stakedAt DOES NOT get reset (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L401)
4. The user gets issued an irrevocable token; he does not get "upgraded" (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L340)
5. The user now gets burned his irrevocable token; his stakedAt does not get reset (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L725)
6. The user now CAN IMMEDIATELY claim a revocable Prime token (considering he still stakes the minimum staking amount and the difference between his stakedAt and the current block timestamp is bigger than the minimum staking period; https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L399)
7. The user DOES NOT LOSE any staking rewards since he can replace his burned irrevocable Prime token immediately with a revocable Prime token through calling claim() again.

II). User that did claim is at disadvantage

1. User has staked the minimum amount of XVS tokens to be eligible for claiming a revocable prime token by calling claim() in Prime.sol (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L397).
2. His stakedAt time is set to the timestamp when the first time xvsUpdated() was called after he staked the minimum XVS amount (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L378). The minimum staking period of 90 days has passed.
3. The user DOES call claim() so he gets a revocable prime token; his stakedAt DOES reset (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L401)
4. The user gets issued an irrevocable token; he gets "upgraded" (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L338)
5. The user now gets burned his irrevocable token; his stakedAt was already reset before and remains reset (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L725)
6. The user now CANNOT claim a revocable Prime token (although he still stakes the minimum staking amount because the difference between his stakedAt and the current block timestamp is 0; https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L399)
7. The user DOES LOSE 90 days staking rewards (duration of the minimum staking period) since he cannot replace his burned irrevocable Prime token immediately with a revocable Prime token through calling claim() again.

Summary

Both users have an almost identical scenario. The only difference is that the user in scenario I. did not claim a revocable token where the user in scenario II. did claim a revocable token. Both had achieved and did hold the requirements to be eligible for claiming and holding a revocable prime token while they got an irrevocable prime token issued and then burned.

Nevertheless the user that was upgraded from revocable to an irrevocable token and then got the token burned lost his initial irrevocable token and needs to stake again for the minimum staking duration and claim a new revocable token to participate in prime staking rewards.

This puts him at a heavy disadvantage which comparing the scenarios is unfair.

Tools Used

Manual review

Recommended Mitigation Steps

I suggest that both scenarios are treated equally. I see 2 options.

Option 1) Always reset the stakedAt of a user whenever issuing an irrevocable (or revocable) token to him. After burning the token each user has to start from a stakedAt of 0. He needs to stake again for the minimum staking period and claim a new revocable token to earn prime rewards again.

Option 2) As a counterpart to the "upgrade" (irrevocable to revocable) when issuing a token, implement a "downgrade" function which allows the user to keep his revocable token he was upgraded from. He does not need to stake again for the minimum staking period and claim a new revocable token to earn prime rewards again.

Coded proof of concept

diff --git a/Prime.ts.orig b/Prime.ts
index f27ce3f..0f6c418 100644
--- a/Prime.ts.orig
+++ b/Prime.ts
@@ -401,6 +401,111 @@ describe("PrimeScenario Token", () => {
       expect(token.exists).to.be.equal(false);
     });

+    describe.only("POC", () => {
+      it("user that did not claim is at advantage", async () => {
+        const user = user1;
+        const MINIMUM_STAKED_XVS = 1000;
+        const MINIMUM_STAKING_TIME = 90 * 24 * 60 * 60;
+
+        // check that user cannot claim
+        await expect(prime.connect(user).claim()).to.be.revertedWithCustomError(prime, "IneligibleToClaim");
+
+        // user deposits minimum XVS amount to be eligible for a revocable token
+        await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(MINIMUM_STAKED_XVS));
+        await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(MINIMUM_STAKED_XVS));
+
+        // the clock for 90 days gets started
+        await prime.xvsUpdated(user.getAddress());
+        const stakedAt = await prime.stakedAt(user.getAddress());
+        const timestamp = (await (ethers.provider.getBlock('latest'))).timestamp -1;
+        expect(stakedAt).to.equal(timestamp);
+
+        // irrevocable token gets issued
+        const irrevocable = true;
+        await prime.issue(irrevocable, [user.getAddress()]);
+        const issuedToken = await prime.tokens(user.getAddress())
+        expect(issuedToken.exists).to.be.true;
+        expect(issuedToken.isIrrevocable).to.equal(irrevocable);
+
+        // 90 days pass
+        await mine(MINIMUM_STAKING_TIME);
+
+        // irrevocable token gets burned
+        prime.burn(user.getAddress());
+        const revokedToken = await prime.tokens(user.getAddress())
+        expect(revokedToken.exists).to.be.false;
+        expect(revokedToken.isIrrevocable).to.be.false;
+
+        // stakedAt is still the initially set stakedAt
+        const stakedAtAfterBurning = await prime.stakedAt(user.getAddress());
+        expect(stakedAtAfterBurning).to.equal(stakedAt);
+
+        // revocable token gets immediately claimed by user
+        await prime.connect(user).claim();
+        const claimedToken = await prime.tokens(user.getAddress())
+        expect(claimedToken.exists).to.be.true;
+        expect(claimedToken.isIrrevocable).to.be.false;
+      });
+
+      it("user that did claim is at disadvantage", async () => {
+        const user = user1;
+        const MINIMUM_STAKED_XVS = 1000;
+        const MINIMUM_STAKING_TIME = 90 * 24 * 60 * 60;
+
+        // check that user cannot claim
+        await expect(prime.connect(user).claim()).to.be.revertedWithCustomError(prime, "IneligibleToClaim");
+
+        // user deposits minimum XVS amount to be eligible for a revocable token
+        await xvs.connect(user).approve(xvsVault.address, bigNumber18.mul(MINIMUM_STAKED_XVS));
+        await xvsVault.connect(user).deposit(xvs.address, 0, bigNumber18.mul(MINIMUM_STAKED_XVS));
+
+        // 90 days pass
+        await mine(90 * 24 * 60 * 60);
+
+        // revocable token gets claimed by user
+        await prime.connect(user).claim();
+        const claimedToken1 = await prime.tokens(user.getAddress())
+        expect(claimedToken1.exists).to.be.true;
+        expect(claimedToken1.isIrrevocable).to.be.false;
+
+        // the clock for 90 days gets reset to 0
+        await prime.xvsUpdated(user.getAddress());
+        const stakedAt = await prime.stakedAt(user.getAddress());
+        expect(stakedAt).to.equal(0);
+
+        // irrevocable token gets issued (token gets upgraded)
+        const irrevocable = true;
+        await prime.issue(irrevocable, [user.getAddress()]);
+        const issuedToken = await prime.tokens(user.getAddress())
+        expect(issuedToken.exists).to.be.true;
+        expect(issuedToken.isIrrevocable).to.equal(irrevocable);
+
+        // irrevocable token gets burned
+        prime.burn(user.getAddress());
+        const revokedToken = await prime.tokens(user.getAddress())
+        expect(revokedToken.exists).to.be.false;
+        expect(revokedToken.isIrrevocable).to.be.false;
+
+        // stakedAt is still 0
+        const stakedAtAfterBurning = await prime.stakedAt(user.getAddress());
+        expect(stakedAtAfterBurning).to.equal(stakedAt);
+
+        // revocable token cannot get immediately claimed
+        const claimTime = (await prime.claimTimeRemaining(user.getAddress())).toNumber();
+        expect(claimTime).to.equal(MINIMUM_STAKING_TIME);
+
+        // again 90 days pass
+        await prime.xvsUpdated(user.getAddress());
+        await mine(MINIMUM_STAKING_TIME);
+
+        // revocable token gets claimed by user after waiting time
+        await prime.connect(user).claim();
+        const claimedToken2 = await prime.tokens(user.getAddress())
+        expect(claimedToken2.exists).to.be.true;
+        expect(claimedToken2.isIrrevocable).to.be.false;
+      });
+    });

Assessed type

Other

L-01 Calling _claimInterest() function in Prime.sol re-assigns user's reward index after it was set to 0 when their token was burned

Burning a token sets the user's score and reward index and score to 0 (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L736-L737). Nevertheless after having a token burned the user can still claim the rewards by calling one of the claimInterest functions in Prime.sol (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L433, https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L443).

These functions don't check whether the user still holds a prime token. They also re-assign the user the respective market reward index (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L676) so the users reward index is not more 0.

I have not found a way to exploit this. Potentially this will skew reward calculation for the user once he claims a prime token again. This should be further explored.

L-02 updateScores() function in Prime.sol reverts when token of user in passed users array is burned prior to calling this function (frontrun)

The function already implements continue instead of revert if the token of a user was already updated in the current update round id (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L208). This prevents the function to revert if the user was updated in a previous call or is contained more than once in the passed users array.

But the function reverts when the Prime token of a user does not exist (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L207). This can also happen when a user's token gets burned in the same block (unstakes under minimum staking limit, gets burned manually via protected and public burn function). This could be considered a frontrun.

Under certain circumstance this could even be used to DoS the score updates for a group of users (the passed array of users for which the transaction reverts). A user contained in the users array would need to observe the public mempool and frontrun the updateScores() call to not have an existing prime token anymore.

Since getting a prime token in the first place (at least through self-minting) requires a minimum stake of 1000 XVS (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeStorage.sol#L34; ~$5000 at current $5/XVS token) and tokens cannot immediately be restaked (needs 90 days minimum staking period) this seems not very likely to be used for DoS but it is certainly possible.

To solve the explained issue the updateScores () function could implement the following instead;

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

L-03 Prime.sol issue() issue function would be more versatile using an array parameter for isIrrevocable

Changing the interface of the issue() function (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L331) to the following would allow issuing irrevocable and revocable tokens with just 1 function call:

function issue(bool[] isIrrevocable, address[] calldata users) external {}

It would be necessary though to validate that both arrays have the same length. Using 2 arrays (one for contract/EOA addresses) and another for matching a parameter for each address entry is a common pattern and could be implemented here.

L-04 Comment for isIrrevocable param of Prime.sol issue() function seems wrong

The tokens are "issued" in either case independent of whether it is an irrevocable token or a revocable token (https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L328). Comment should rather be: "Are issued tokens irrevocable ones" or "Issued tokens are irrevocable".

L-05 Inconsistent pattern used for resetting stakedAt to 0 in Prime.sol

In Prime.sol the stakedAt of a user is reset to 0 in more than 1 place. But different patterns are used to reset the value to 0. In https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L352 the value is deleted. In https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L376 the value is assign 0. One of both patterns should be used exclusively to reach consistency.

L-06 Renaming variables and slight restructuring would give accrueInterest(address vToken) function in Prime.sol more clarity

The "distributionIncome" variable (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L568) should better be renamed to sth. like "unreleasedIncome" or "unreleasedPrimeIncome". The following would give the function more clarity (at the expense of 1 additional local variable:

uint256 unreleasedPSRIncome = totalIncomeUnreleased - unreleasedPSRIncome[underlying];
...
uint256 totalAccruedInPLP = _primeLiquidityProvider.tokenAmountAccrued(underlying);
uint256 unreleasedPLPAccruedInterest = totalAccruedInPLP - unreleasedPLPIncome[underlying];

uint256 unreleasedIncome = unreleasedPSRIncome + totalAccruedInPLP;

L-07 local isMarketExist variable inside of addMarket() function of Prime.sol should be renamed

It feels like the naming is grammatically wrong. The variable should be renamed to sth. like "isMarketExisting", "doesMarketExist", "marketExisting", "marketExists" here https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L292.

L-08 _executeBoost has wrong comment indicating it needs to be called "before changing account's borrow or supply balance."

The comment (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L775) should be "Update total score of user and market. Must be called after changing account's borrow or supply balance." like commented for the _updateScore function (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L790).

L-09 TokenDistributionSpeedUpdated event in PrimeLiquidityProvider.sol could also log the old distribution speed

The PrimeLiquidityProvider event only logs the new speed (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L33) whereas the PrimeTokenUpdated event also logs the old prime token when logging the new prime token (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L36). Logging the old and the new value for all "*Updated" events could be a pattern worth following throughout the codebase.

L-10 TokenNotInitialized event in PrimeLiquidityProvider.sol uses parameter with trailing underscore

TokenNotInitialized uses the "token_" parameter with trailing underscore (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L75). Other events use the "token" parameter without underscore (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L66). The underscore should be removed.

L-11 The tokens_ parameter of initialize() function in PrimeLiquidityProvider.sol could be enhanced with a duplication check

Prior to iterating the passed tokens array or while iterating it (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L103) a duplication check could be introduced making sure a token is not processed multiple times. This would reduced the risk that a token is included more than once but with varying distribution speed which would lead to overwrites.

L-12 Incrementing the index in loops is not uniform across contracts

For example here "++1" is used (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L108) and here "i++" is used (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/Prime.sol#L189).

L-13 The setTokensDistributionSpeed() function should be moved within PrimeLiquidityProvider.sol

It should be placed under the initializeTokens() functions as both functions are required for adding new tokens (same context). I recommend also moving the pauseFundsTransfer() and resumeFundsTransfer() functions (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L132-L144) to the bottom of the contract. They are a dedicated aspect and should not be put between other contract aspects concerned with funds, rewards, etc..

L-14 It would be beneficial to have a check for not sweeping any of the accrued tokens for sweep() function in PrimeLiquidityProvider.sol

The sweep() function (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L216) allows sweeping the full balance of the contract. It only reverts for an attempt to sweep more than the contract balance (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L218). By accident the balance could be corrected to an amount that does not cover the accrued tokens in the PLP anymore. The would otherwise lead to an underflow here (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L261) and a revert trying to send more tokens than available here (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L204).

L-15 tokenAccrued variable in accrueTokens() functions of PrimeLiquidityProvider.sol could be written a bit cleaner

Currently the following code is used (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L264):

uint256 tokenAccrued = (balanceDiff <= accruedSinceUpdate ? balanceDiff : accruedSinceUpdate);

It could be rewritten (semantically) cleaner to use < instead of <=.

uint256 tokenAccrued = (balanceDiff < accruedSinceUpdate ? balanceDiff : accruedSinceUpdate);

L-16 Some NatSpec comments start with a lowercase some with an uppercase letter

Example for lowercase: https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeStorage.sol#L87 Example for uppercase: https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeStorage.sol#L60. Should, be consistent throughout the codebase.

L-17 lessxvsThanCapital variable in Scores.sol does is nor correctly formatted.

The variable should be written "lessXvsThanCapital" instead of "lessxvsThanCapital" (https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/libs/Scores.sol#L52)

L-18

Comment for lastAccruedBlock in PrimeLiquidityProvider seems wrong https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L23. It is rather "The block the last time token were accrued" instead of "The rate at which token is distributed to the Prime contract". Most likely this is a partial copy paste issue from here: https://github.com/code-423n4/2023-09-venus/blob/main/contracts/Tokens/Prime/PrimeLiquidityProvider.sol#L20. Should be corrected.