Venus Protocol Isolated Pools - sashik_eth's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/WhitePaperInterestRateModel.sol#L17

Vulnerability details

Rate model contracts BaseJumpRateModelV2 and WhitePaperInterestRateModel both include a blocksPerYear constant that corresponds to the number of blocks produced on the chain during 1 year. This constant is used to calculate immutables variables that influence borrow and supply rates:

File: WhitePaperInterestRateModel.sol
31:     /**
32:      * @notice Construct an interest rate model
33:      * @param baseRatePerYear The approximate target base APR, as a mantissa (scaled by BASE)
34:      * @param multiplierPerYear The rate of increase in interest rate wrt utilization (scaled by BASE)
35:      */
36:     constructor(uint256 baseRatePerYear, uint256 multiplierPerYear) {
37:         baseRatePerBlock = baseRatePerYear / blocksPerYear;
38:         multiplierPerBlock = multiplierPerYear / blocksPerYear;
39: 
40:         emit NewInterestParams(baseRatePerBlock, multiplierPerBlock);
41:     }

Venus protocol is deployed on the BSC chain, where a new block produces every 3 seconds in comparison to Ethereum 12 seconds. This means that on BSC chain during the 1 year produces approximately 10512000 new blocks:

365 * 24 * 3600 / 3 = 10512000

While in BaseJumpRateModelV2.sol used the correct value for blocksPerYear constant:

File: BaseJumpRateModelV2.sol
20:     /**
21:      * @notice The approximate number of blocks per year that is assumed by the interest rate model
22:      */
23:     uint256 public constant blocksPerYear = 10512000;

In WhitePaperInterestRateModel.sol we can see the incorrect value left from the original Compound codebase:

File: WhitePaperInterestRateModel.sol
14:     /**
15:      * @notice The approximate number of blocks per year that is assumed by the interest rate model
16:      */
17:     uint256 public constant blocksPerYear = 2102400; 

Impact

An incorrect number of blocks per year would directly influence on supply and borrow rates leading to wrong computation for suppliers and borrowers. Since the block variable is constant - redeploying and resetting of rate model contract would be required to mitigate the problem in existing markets. Preventing new markets with the incorrect constant protocol would require an upgrade of existing core contracts since PoolRegistry use a constant factory that deploys WhitePaperInterestRateModel instances with incorrect constant.

Proof of Concept

Next scenario possible:

1. Developers deploy contact with the incorrect hardcoded values of blocks per year.
2. Admin creates the new isolated pool and adds market with WhitePaperInterestRateModel as rate model.
3. Users supply and borrow tokens with incorrect rates due to wrong calculations based on incorrect blocks per year number.

Recommended Mitigation Steps

Update blocksPerYear value in WhitePaperInterestRateModel to correct one - 10512000

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Shortfall/Shortfall.sol#L183 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Shortfall/Shortfall.sol#L248

Vulnerability details

Auction logic in Shortfall contract sending previous bid assets to the previous bidder during placing a new bid:

File: Shortfall.sol
179:             if (auction.auctionType == AuctionType.LARGE_POOL_DEBT) {
180:                 if (auction.highestBidder != address(0)) {
181:                     uint256 previousBidAmount = ((auction.marketDebt[auction.markets[i]] * auction.highestBidBps) /
182:                         MAX_BPS);
183:                     erc20.safeTransfer(auction.highestBidder, previousBidAmount); 
184:                 }
185: 
186:                 uint256 currentBidAmount = ((auction.marketDebt[auction.markets[i]] * bidBps) / MAX_BPS);
187:                 erc20.safeTransferFrom(msg.sender, address(this), currentBidAmount); 
188:             } else {
189:                 if (auction.highestBidder != address(0)) {
190:                     erc20.safeTransfer(auction.highestBidder, auction.marketDebt[auction.markets[i]]); 
191:                 }

This approach breaks "Pull over Push" pattern: https://fravoll.github.io/solidity-patterns/pull_over_push.html

While it would work correctly for regular ERC20 tokens, for tokens with callback calls to the receiver (ERC777) - this open DOS attack vector. Attackers would be able to place their own bid and after this prevent new bids from another user by reverting any transfer to their own address.

Impact

Attacker could prevent new bids in the auction, leading to non-optimal results for protocol when the winning bid is not the most profitable.

In the same way, closeAuction() could be DOS in case if convertibleBaseAsset token would have a callback to the receiver. In this case, an attacker would not have direct profit but the flow of the auction would be blocked.

Proof of Concept

Next scenario possible:

1. Pool acquires some badDebt amount, one of the markets in this pool has ERC777 token as underlying.
2. Creating an auction in Shortfall contract for this pool.
3. Attacker deploys a contract that reverts any incoming transfer of ERC777. Making a bid using this contract.
4. New user trying to place "better" bid, but tx reverts each time due to a revert in callback on attacker contract.
5. Attacker wins the auction.

Recommended Mitigation Steps

Consider using a "pull over push" pattern for transferring previous bids and prizes. In this way, the previous bidder would not be able to prevent new bids. Previous bids and prizes would be stored on the contract until bidders do not withdraw by themselves.

Assessed type

Token-Transfer

Summary

Low Risk Issues List

Non-Critical Issues List

---

L-01 MaxLoopsLimitHelper is inefficient

Multiple times in protocol there are calls to MaxLoopsLimitHelper#_ensureMaxLoops function that is designed to avoid DOS fails:

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/MaxLoopsLimitHelper.sol#L39

File: MaxLoopsLimitHelper.sol
34:     /**
35:      * @notice Comapre the maxLoopsLimit with number of the times loop iterate
36:      * @param len Length of the loops iterate
37:      * @custom:error MaxLoopsLimitExceeded error is thrown when loops length exceeds maxLoopsLimit
38:      */
39:     function _ensureMaxLoops(uint256 len) internal view {
40:         if (len > maxLoopsLimit) {
41:             revert MaxLoopsLimitExceeded(maxLoopsLimit, len);
42:         }
43:     }

However, instead of preventing such failures, this functionality would only increase cases when DOS occurs. For example, in the function RewardsDistributor#claimRewardToken():

File: RewardsDistributor.sol
277:     function claimRewardToken(address holder, VToken[] memory vTokens) public {
278:         uint256 vTokensCount = vTokens.length;
279: 
280:         _ensureMaxLoops(vTokensCount);
281: 
282:         for (uint256 i; i < vTokensCount; ++i) {
283:             VToken vToken = vTokens[i];
284:             require(comptroller.isMarketListed(vToken), "market must be listed");
285:             Exp memory borrowIndex = Exp({ mantissa: vToken.borrowIndex() });
286:             _updateRewardTokenBorrowIndex(address(vToken), borrowIndex);
287:             _distributeBorrowerRewardToken(address(vToken), holder, borrowIndex);
288:             _updateRewardTokenSupplyIndex(address(vToken));
289:             _distributeSupplierRewardToken(address(vToken), holder);
290:         }
291:         rewardTokenAccrued[holder] = _grantRewardToken(holder, rewardTokenAccrued[holder]);
292:     }

If DOS would occur with vTokensCount greater than 50 and current maxLoopsLimit is 40, then DOS would take place inside MaxLoopsLimitHelper#_ensureMaxLoops on any vTokensCount on any value > 40. While it would be fine up to 50 if MaxLoopsLimitHelper wasn't used.

Recommendation: Consider removing MaxLoopsLimitHelper functionality in places where it only provokes DOS.

L-02 decimals() function is not guaranteed in ERC20

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Pool/PoolRegistry.sol#L284

File: PoolRegistry.sol
284:         uint256 underlyingDecimals = IERC20Metadata(input.asset).decimals();

According to the EIP-20, the decimals() function is considered optional for ERC-20 tokens, meaning that tokens without this function may be unable to participate in certain protocol functionalities. Recommendation: Consider updating function that calls decimals() function.

L-03 Miscalculated gap size

The conventional size for the "gap" of unused storage slots, reserved for new variables is 50 minus the number of slots already used. In multiple places in scope this convention is broken:

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/ReserveHelpers.sol#L26 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/ComptrollerStorage.sol#L122 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VTokenInterfaces.sol#L130 Recommendation: Consider updating the "gap" size to appropriate numbers.

L-04 Anyone could claim rewards on others' behalf

RewardsDistributor#claimRewardToken() allows claim rewards tokens with arbitrary holder address:

File: RewardsDistributor.sol
237:     /**
238:      * @notice Claim all the rewardToken accrued by holder in all markets.
239:      * @param holder The address to claim REWARD TOKEN for
240:      */
241:     function claimRewardToken(address holder) external { 
242:         return claimRewardToken(holder, comptroller.getAllMarkets());
243:     }

While it's not lead to direct loss of funds, it could be used for griefing attacks in terms of taxes for example by forcing claim rewards when user don't interested in that. Recommendation: Consider allowing reward claiming only for own msg.sender.

L-05 The owner could bypass withdraw check in case of multi-address token

Token#sweepToken allows the contract owner to withdraw ERC-20 tokens accidentally sent to the contract address. This function includes check that the withdrawing token is not an underlying, preventing potential rug-pull:

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L526

File: VToken.sol
519:     /**
520:      * @notice A public function to sweep accidental ERC-20 transfers to this contract. Tokens are sent to admin (timelock)
521:      * @param token The address of the ERC-20 token to sweep
522:      * @custom:access Only Governance
523:      */
524:     function sweepToken(IERC20Upgradeable token) external override { 
525:         require(msg.sender == owner(), "VToken::sweepToken: only admin can sweep tokens");
526:         require(address(token) != underlying, "VToken::sweepToken: can not sweep underlying token");
527:         uint256 balance = token.balanceOf(address(this));
528:         token.safeTransfer(owner(), balance);
529: 
530:         emit SweepToken(address(token));
531:     }

However, this check would be inefficient for multi-address tokens, since it checks only one address and the owner could use the other one for a call and withdraw all underlying balances.

Recommendation: Consider comparing the balance of underlying tokens before and after the sweep, this would prevent withdrawing underlying tokens with multiple addresses.

N-01 Wrong/misleading comments

Comment refers to other varialbe:

File: PoolRegistry.sol
75:     /**
76:      * @notice Shortfall contract address 
77:      */
78:     address payable public protocolShareReserve;

Comments inform about possible transfers, while functions don't call transfer():

File: RewardsDistributor.sol
335:     /**
336:      * @notice Calculate REWARD TOKEN accrued by a supplier and possibly transfer it to them. 
337:      * @param vToken The market in which the supplier is interacting
338:      * @param supplier The address of the supplier to distribute REWARD TOKEN to
339:      */
340:     function _distributeSupplierRewardToken(address vToken, address supplier) internal {

File: RewardsDistributor.sol
369:     /**
370:      * @notice Calculate reward token accrued by a borrower and possibly transfer it to them.
371:      * @param vToken The market in which the borrower is interacting
372:      * @param borrower The address of the borrower to distribute REWARD TOKEN to
373:      */
374:     function _distributeBorrowerRewardToken(

N-02 Redundant check

No needed zero check here, since address would be checked during OZ UpgradeableBeacon constructor call: https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Proxy/UpgradeableBeacon.sol#L8

File: UpgradeableBeacon.sol
7:     constructor(address implementation_) UpgradeableBeacon(implementation_) {
8:         require(implementation_ != address(0), "Invalid implementation address"); 
9:     }