Venus Prime - tapir's results

Lines of code

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

Vulnerability details

Impact

If protocol admin issues an user an irrevocable prime token it does not check if the users "stakedAt" is already started. When the admin decides to remove the user from the revocable token holders and if user gets his token burnt for some reason user will be able to claim the next token without waiting any time because the "stakedAt" was already counting when he had the prime token. This will lead some users to bypass the waiting time for minting a new token because they had already the time ticking while they had prime tokens already.

Proof of Concept

Assume Alice will be rewarded a irrevocable prime token by the protocol. She knows that and quickly deposits some XVS to XVS Vault which she has enough XVS tokens hence, this line will be executed for Alice: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L377-L378 As we can see above snippet, Alice "stakedAt[Alice]" will be start ticking.

When the "issue" function is called by the protocol admin these lines will be executed because the token is irrevocable: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L331-L348 Which we can see that it does not checks whether the user has already a started "stakedAt" or not.

Then once the Alice gets his irrevocable token via protocol admins "issue" call Alice has already a "stakedAt" time counting. If for some reason Alice's irrevocable token gets burnt by the admin Alice can claim a revocable token immediately assuming the time is already passed for Alice and Alice still holds required XVS in the vault to claim a revocable prime token.

Tools Used

Recommended Mitigation Steps

Add this to the above part of the if statement https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L352

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L647-L664 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L872-L897

Vulnerability details

Impact

Scores are fixed always in 18 decimals. However, if the underlying token decimals and the vToken of it not sharing the same decimals than the scores will be calculated in different decimals than 18. In this case the calculating of boost will be trouble because scores are assumed to be in 18 decimals always. All the math operations on scores are executed as considering the score has 18 decimals which will lead to accounting problems if scores are not in 18 decimals.

Proof of Concept

Here are the two code snippets we will be looking at: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L647-L664 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L872-L897

Here the code snippet from my end commenting every variables decimals and showing the problem:

1:

function _calculateScore(address market, address user) internal returns (uint256) {
        // @audit Decimals: 18
        uint256 xvsBalanceForScore = _xvsBalanceForScore(_xvsBalanceOfUser(user));

        IVToken vToken = IVToken(market);

        // @audit Decimals: underlyingTokenDecimals
        uint256 borrow = vToken.borrowBalanceStored(user); // @audit this is stored not the latest interest accounted!

        // @audit Decimals: 18 - 8 + underlyingTokenDecimals
        uint256 exchangeRate = vToken.exchangeRateStored(); // @audit this is stored not the latest interest accounted!

        // @audit Decimals: cToken decimals, 8.
        uint256 balanceOfAccount = vToken.balanceOf(user);

        // (18 - 8 + underlyingTokenDecimals + cTokenDecimals) - 18 =
        // = underlyingTokenDecimals + cTokenDecimals - 8 =
        // @audit Decimals: underlyingTokenDecimals, considering cTokenDecimals == 8
        uint256 supply = (exchangeRate * balanceOfAccount) / EXP_SCALE; 

        address xvsToken = IXVSVault(xvsVault).xvsAddress();
        oracle.updateAssetPrice(xvsToken);
        oracle.updatePrice(market);

        // @audit Decimals: underlyingTokenDecimals
        (uint256 capital, , ) = _capitalForScore(xvsBalanceForScore, borrow, supply, market);

        // @audit This should be...
        // capital * (10 ** (18 - underlyingToken.decimals())) !!
        // because capital is in underlyingTokenDecimals not vToken decimals.
        capital = capital * (10 ** (18 - vToken.decimals()));

        return Scores.calculateScore(xvsBalanceForScore, capital, alphaNumerator, alphaDenominator);
    }

2:

function _capitalForScore(
        uint256 xvs, // @audit 18 decimals
        uint256 borrow, // @audit in underlying token decimals
        uint256 supply, // @audit in underlying token decimals
        address market
    ) internal view returns (uint256, uint256, uint256) {
        address xvsToken = IXVSVault(xvsVault).xvsAddress();

        // @audit Decimals: 36 - underlyingTokenDecimals
        uint256 xvsPrice = oracle.getPrice(xvsToken);

        // @audit Decimals: 18 * (18 * 18 / 18) / 18 = 18 decimals always, both of these
        uint256 borrowCapUSD = (xvsPrice * ((xvs * markets[market].borrowMultiplier) / EXP_SCALE)) / EXP_SCALE;
        uint256 supplyCapUSD = (xvsPrice * ((xvs * markets[market].supplyMultiplier) / EXP_SCALE)) / EXP_SCALE;

        // @audit Decimals: 36 - underlyingTokenDecimals
        uint256 tokenPrice = oracle.getUnderlyingPrice(market);

        // @audit Decimals: (36 - underlyingTokenDecimals + underlyingTokenDecimals) - (18) = 18 decimals
        uint256 supplyUSD = (tokenPrice * supply) / EXP_SCALE; 
        uint256 borrowUSD = (tokenPrice * borrow) / EXP_SCALE;  

        if (supplyUSD >= supplyCapUSD) {
            // @audit Decimals: underlyingTokenDecimals
            supply = supplyUSD > 0 ? (supply * supplyCapUSD) / supplyUSD : 0;
        }

        if (borrowUSD >= borrowCapUSD) {
            // @audit Decimals: underlyingTokenDecimals
            borrow = borrowUSD > 0 ? (borrow * borrowCapUSD) / borrowUSD : 0;
        }

        // @audit Decimals: all of them are in underlyingTokenDecimals
        return ((supply + borrow), supply, borrow);
    }

Now let's trace the comments here aswell. Before that remember the oracle returns the decimals as (36-underlyingTokenDecimals) as sponsor stated. Now, first "_capitalForScore(xvsBalanceForScore, borrow, supply, market)" the inputs borrow and supply are in terms of the underlying token of the vToken as we can see in the code snippets above with the explanations. And inside the "_capitalForScore" the return values are in underlyingTokenDecimals. However, here in this line: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L661 the return value is multiplied with the vTokens decimals which is wrong.

Assume a token with 6 decimals and its vToken has 8 decimals. In this case the capital will be in terms of 6 decimals and when it comes to that line it will scaled up to 1e6 * 10 ** (18 - 8) = 1e16 which is not 18 decimals.

Here are the lines where the calculations will be wrong because of the scores are assumed to be in 18 decimals: https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L585C92-L585C92 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L922

Tools Used

Recommended Mitigation Steps

Change this line: "capital = capital * (10 ** (18 - vToken.decimals()));" to: "capital = capital * (10 ** (18 - underlyingToken.decimals()));"

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

When updateScores is necessary to call it does a for loop for given users and decrease the pending score updates and updates the scores of the previous users. However, if the loops "continue" is ever triggered than the function will spend all of its gas because the "i" will not be incremented. This will lead to massive amount of gas lost for the caller. An attacker can grief the system by frontrunning the updateScores function call which would make the victim spend all the gas without executing the loop logic.

Proof of Concept

Assume that the alpha changed and there were 100 users in total (irrevocable and revocable) hence, the "pendingScoreUpdates" variable is set to 100. https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L254 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L818-L822

Protocol admin wants to update the 100 users and prepares the address[] array.

Malicious user spots the function call in the mempool and quickly calls the "updateScores" function with only 1 address in its array input where as that address is the protocol admins input arrays 0th index.

When the malicious users tx is executed the admins tx will take place. When it hits to these lines it will get in to a infinite loop and it will not stop untill all the gas is spent because the "i" will not be incremented when "continue" is executed in the loop. https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L208 This will make anyone calling this function hesitated because malicious user can keep doing this for anyone try to call this function.

Tools Used

Recommended Mitigation Steps

Increment the "i" if "continue" will be triggered inside the loop such that the function can keep looping the inputs without getting into a infinite loop.

Assessed type

Loop