Venus Protocol Isolated Pools - Audit_Avengers_2's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Shortfall/Shortfall.sol#L179-L194

Vulnerability details

Impact

Impact is High. If erc777 tokens are added to the Vtoken contract as underlying, then that would enable malicious actor to brick the bidding process indefinitely by making a bid via malicious contract. once a valid bidder outbids him, the placebid function would transfer back the malicious bidders tokens, which could just keep reverting via the erc777 tokensreceived hook, ergo no one else can place any bids any longer.

Severity is Low since im assuming Vtokens added to a pool are done by owners/maintainers of the protocol, so they can validate beforehand whether underlying is erc777 or not, and only add underlyings that dont fire a transfer hook.

Proof of Concept

            if (auction.auctionType == AuctionType.LARGE_POOL_DEBT) {
            if (auction.highestBidder != address(0)) {
                uint256 previousBidAmount = ((auction.marketDebt[auction.markets[i]] * auction.highestBidBps) /
                    MAX_BPS);
            //@audit if token is erc777, malicious actor can just revert transaction via hook    
                erc20.safeTransfer(auction.highestBidder, previousBidAmount);
            }

            uint256 currentBidAmount = ((auction.marketDebt[auction.markets[i]] * bidBps) / MAX_BPS);
            erc20.safeTransferFrom(msg.sender, address(this), currentBidAmount);
        } else {
            if (auction.highestBidder != address(0)) {
                //@audit if token is erc777, malicious actor can just revert transaction via hook
                erc20.safeTransfer(auction.highestBidder, auction.marketDebt[auction.markets[i]]);
            }

            erc20.safeTransferFrom(msg.sender, address(this), auction.marketDebt[auction.markets[i]]);
        }

As you can see from code above, the placebid function attempts to return the bidded tokens to the previous bidder, but if the underlying is an erc777 and the previous bidder is a contract, they can just force the hook to revert always, thereby disabling the ability to place any bids by anyone.

Tools Used

Recommended Mitigation Steps

Do not add erc777 or another other tokens that fire hooks on transfer. this could be also validated by a helper method for instance that is called when the underlying is sanity checked within the Vtoken contract.

// Set underlying and sanity check it
underlying = underlying_;
IERC20Upgradeable(underlying).totalSupply();
require(!isERC777(underlying), "ERC777 tokens are not allowed"); //@audit 
    

Assessed type

DoS