Venus Prime - nobody2018's results

Lines of code

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

Vulnerability details

Impact

AccrueTokens internally may revert due to underflow in some cases. This function is called in Prime.sol, which will cause some important processes to fail. Venus is a lending protocol that is very time-sensitive. Any temporary DOS will have a negative impact on all participants of Venus.

Proof of Concept

Let’s first take a look at the important processes mentioned above:

XVSVault.deposit/requestWithdrawal
  Prime.xvsUpdated
    _accrueInterestAndUpdateScore
      _executeBoost
        accrueInterest
          PrimeLiquidityProvider.accrueTokens	//revert

//in Comptroller
PolicyFacet.mintVerify/redeemVerify/borrowVerify/repayBorrowVerify/liquidateBorrowVerify/seizeVerify/transferVerify
  Prime.accrueInterestAndUpdateScore
    _executeBoost
      accrueInterest
        PrimeLiquidityProvider.accrueTokens   //revert

From the above processes, we can see that if accumulateTokens revert, the impact will be significant.

File: contracts\Tokens\Prime\PrimeLiquidityProvider.sol
249:     function accrueTokens(address token_) public {
......
254:         uint256 blockNumber = getBlockNumber();
255:         uint256 deltaBlocks = blockNumber - lastAccruedBlock[token_];
256: 
257:         if (deltaBlocks > 0) {
258:             uint256 distributionSpeed = tokenDistributionSpeeds[token_];
259:             uint256 balance = IERC20Upgradeable(token_).balanceOf(address(this));
260: 
261:->           uint256 balanceDiff = balance - tokenAmountAccrued[token_];
262:             if (distributionSpeed > 0 && balanceDiff > 0) {
263:                 uint256 accruedSinceUpdate = deltaBlocks * distributionSpeed;
264:                 uint256 tokenAccrued = (balanceDiff <= accruedSinceUpdate ? balanceDiff : accruedSinceUpdate);
265: 
266:                 tokenAmountAccrued[token_] += tokenAccrued;
267:                 emit TokensAccrued(token_, tokenAccrued);
268:             }
269: 
270:             lastAccruedBlock[token_] = blockNumber;
271:         }
272:     }

Under normal circumstances, balance is definitely greater than or equal to tokenAmountAccrued[token_] which is no problem.

We noticed that the sweepToken function is used to sweep accidental ERC-20 transfers to this contract. This is to prevent users from accidentally transferring ERC20 to this contract, and the owner can return the token to the user. However, malicious users can take advantage of this to transfer the accumulated tokens of the contract and request the owner to return the tokens.

• Before the owner calls sweepToken, accumulateTokens has been called, so the value of tokenAmountAccrued[token] already includes the token transferred by the malicious user.
• After sweepToken is called, the token held by this contract must be less than tokenAmountAccrued[token]. This causes subsequent accumulateTokens revert at line L261.

Consider the following scenario:

For simplicity, we assume there is only one token: tokenA.

tokenA.balanceOf(address(this)) = 1000e18
tokenAmountAccrued[tokenA] = 1000e18

1. The malicious user transfers 10e18 tokenA to this contract. Therefore tokenA.balanceOf(address(this)) = 1000e18 + 10e18 = 1010e18.
2. The malicious user contacts the owner to request the return of tokenA, and provided the transfer tx to the owner.
3. Because anyone can call accumulateTokens, the malicious user calls accumulateTokens(tokenA). Therefore tokenAmountAccrued[tokenA] = 1010e18. Note: If accumulateTokens has been triggered by an external process, there is no need to call it specifically.
4. The owner confirmed that tokenA was transferred to this contract by mistake, so he called sweepToken(tokenA, userAddr, 10e18).
5. At this time, tokenA.balanceOf(address(this)) = 1000e18. tokenAmountAccrued[tokenA] = 1010e18. Then, subsequent accumulateTokens called by external processes will revert due to underflow.

This issue discusses the owner's return of tokens that were mistakenly transferred by users, and is not a malicious call by the owner.

Tools Used

Manual Review

Recommended Mitigation Steps

Adding check in sweepToken: whether the balance in the contract will be less than tokenAmountAccrued[token_] after transfer out.

Assessed type

DoS