Venus Prime - peanuts's results

Lines of code

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

Vulnerability details

Impact

2 impacts are present here.

• Firstly, if the protocol actually intends to give revocable tokens to those who do not stake (so there's no 90 days check and users do not need 1000 staked XVS), then anyone can call xvsUpdated() for that user to destroy his revocable tokens.
• Next, if the protocol insists that revocable tokens are only given to those who meet the requirement of 90 days and 1000 staked XVS, then issue() does not check whether the user has staked for 90 days nor have 1000 XVS tokens, and this will affect the total count of totalRevocable.

I believe that the first impact is more likely because if the users are required to stake for 90 days with 1000 XVS, then they can simply just claim there Prime token themselves; there is no need for the protocol to issue a token to them.

Proof of Concept

In the issue() function, the admin can either mint a irrevocable token or a revocable token for a user without any requirements.

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

Once the token is created for a user, if the user does not have 1000 XVS staked, anyone can call xvsUpdated() to destroy the user's newly created Prime token, which should not be the case.

    function xvsUpdated(address user) external {
        uint256 totalStaked = _xvsBalanceOfUser(user);
        bool isAccountEligible = isEligible(totalStaked);
        if (tokens[user].exists && !isAccountEligible) {
            if (tokens[user].isIrrevocable) {
                _accrueInterestAndUpdateScore(user);
            } else {
                _burn(user);
            }
        } else if (!isAccountEligible && !tokens[user].exists && stakedAt[user] > 0) {
            stakedAt[user] = 0;
        } else if (stakedAt[user] == 0 && isAccountEligible && !tokens[user].exists) {
            stakedAt[user] = block.timestamp;
        } else if (tokens[user].exists && isAccountEligible) {
            _accrueInterestAndUpdateScore(user);
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

issue() could be given as a reward for users to bypass the staking duration. In this sense, since duration is not a factor, only the staking limit should still be checked and those who have 0 staked XVS should not be receiving revocable Prime tokens. Make sure that the issue() function checks that the user has at least 1000 XVS staked, otherwise anyone can just burn their Prime tokens.

Also, since xvsUpdated() is called by the Vault contract, the function should be marked internal instead of external so that only the vault contract can use it.

-    function xvsUpdated(address user) external {
+    function xvsUpdated(address user) internal {

Assessed type

Other