Venus Prime - twicek'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#L397-L405 https://github.com/code-423n4/2023-09-venus/blob/b11d9ef9db8237678567e66759003138f2368d23/contracts/Tokens/Prime/Prime.sol#L407-L414

Vulnerability details

Summary

When an admin burns the prime token of a user, there is a possibility for this user to claim a new prime token without waiting for the staking period.

Proof of Concept

There are 2 possible ways for a user to get a revocable prime token, if an admin calls issue:

Prime.sol#L331-L359

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++;
                }
            }
        }
    }

Or if the user himself calls claim: Prime.sol#L397-L405

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

There is only one way for a user to get an irrevocable token which is if an admin calls issue for this user.

The problem arises when a user gets an irrevocable prime token through issue while having a set block.timestamp in his stakedAt mapping. In this case the user could mint a prime token itself after waiting the STAKING_PERIOD, but let's say that an admin grants him an irrevocable prime token before he calls claim. Later, if the admin (or community) decides to burn the token of this user, he will be able to mint a new revocable token right away without waiting the STAKING_PERIOD because his stakedAt mapping has not been reset to zero and still has the block.timestamp that has been set prior to the admin granting him the prime token.

PS: Even irrevocable tokens are burnable by the admin, as per the comments:

Prime.sol#L407-L414

    /**
     * @notice For burning any prime token
     * @param user the account address for which the prime token will be burned
     */
    function burn(address user) external {
        _checkAccessAllowed("burn(address)");
        _burn(user);
    }

Impact

Users with an irrevocable token who get their prime tokens burned by an admin can bypass the staking period if they have a positive stakedAt at the time of issuance.

Tool used

Manual Review

Recommended Mitigation Steps

Consider deleting the stakedAt also for irrevocable prime token issuance.

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]);
+                   delete stakedAt[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++;
                }
            }
        }
    }

Assessed type

Other

[L-01] No remove market function

Vulnerability Details

Markets can be added through addMarket but cannot be removed. Over time the allMarkets array length might become too high to perform certain calls like _accrueInterestAndUpdateScore:

Prime.sol#L607-L617

    function _accrueInterestAndUpdateScore(address user) internal {
        address[] storage _allMarkets = allMarkets;
        for (uint256 i = 0; i < _allMarkets.length; ) {
            _executeBoost(user, _allMarkets[i]);
            _updateScore(user, _allMarkets[i]);

            unchecked {
                i++;
            }
        }
    }

PS: This issue is present in the bot report but with a poor description so I'm adding it here anyway.

Impact

DoS

Recommended Mitigation Steps

Consider adding a remove market function.

[L-02] updateMultipliers does not have bounds checks for supplyMultiplier and borrowMultiplier

Vulnerability Details

See title.

Prime.sol#L263-L280

    function updateMultipliers(address market, uint256 supplyMultiplier, uint256 borrowMultiplier) external {
        _checkAccessAllowed("updateMultipliers(address,uint256,uint256)");
        if (!markets[market].exists) revert MarketNotSupported();


        accrueInterest(market);


        emit MultiplierUpdated(
            market,
            markets[market].supplyMultiplier,
            markets[market].borrowMultiplier,
            supplyMultiplier,
            borrowMultiplier
        );
        markets[market].supplyMultiplier = supplyMultiplier;
        markets[market].borrowMultiplier = borrowMultiplier;


        _startScoreUpdateRound();
    }

Impact

Having bounds for these parameters will limit the chance of an admin error.

Recommended Mitigation Steps

Consider adding bounds for supplyMultiplier and borrowMultiplier.

[L-03] Consider documenting the process that prevents the non-issue described in the code README under "Potential Locked Tokens"

Vulnerability Details

Potential Locked Tokens According to the logic of function accrueInterest(), it is possible that some rewards will not be collected by >any user. If rewards are currently being issued, but no user has a positive score, then no user can collect >these rewards. Even so, all currently unreleased funds issued can be sent from both PrimeLiquidityProvider and >ProtocolShareReserve to the Prime contract by anyone. Since no user is privy to these funds, they will become >stuck in the contract.

Prime tokens will be issued at the same time (same transaction) the Prime contracts are enabled, so the described scenario will not happen.

It is not clear what enabling Prime contracts means precisely. If it doesn't mean that admins will wait to provide rewards until there is at least one user with a positive score, it will lead to the issue described.

Prime.sol#L583-L586

        uint256 delta;
        if (markets[vToken].sumOfMembersScore > 0) {
            delta = ((distributionIncome * EXP_SCALE) / markets[vToken].sumOfMembersScore);
        }

Impact

If enabling markets solely means to deploy the Prime contract and add markets without having any user with a positive score it could lead to the issue described.

Recommended Mitigation Steps

Consider clarifying in your documentation and in comments the processus allowing to prevent the issue as it's an important vulnerability surface.