Venus Protocol Isolated Pools - Emmanuel's results

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#L190

Vulnerability details

Impact

Adversary will gain RiskFund's assets in an unfair way.

Proof of Concept

In LINE 183 and LINE 190 of Shortfall contract, the protocol tries to send the previous highest bidder's tokens back to him when someone places a higher bid.

erc20.safeTransfer(auction.highestBidder, previousBidAmount);

The problem is, a user can easily make this call to revert and DoS the protocol from sending him his funds, effectively preventing subsequent bidders from bidding. He can do this in two ways:

1. For a vToken with an ERC777 as its underlying token, the user will use a contract to place the bid, and program the callback function of the contract(tokensReceived) to always revert.
2. User could use a blacklisted address to place the bid.

The user will start an auction, place a malicious bid of auction.startBidBPS, then call closeAuction when he can. This would make him gain RiskFund's convertibleBaseAmount at just auction.startBidBPS instead of allowing those who are ready to pay more to win the auction.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider providing a mechanism through which previous highest bidders will manually withdraw their funds. For example:

• Alice places a bid of 100 tokens, which are sent to the contract
• When Bob wants to place a bid of 150 tokens,
  • Bob's tokens should be sent to the contract
  • current highest bidder should be set to Bob
  • try sending Alice tokens back to her, but if it fails, update Alice's amountToWithdraw balance in a mapping
• Alice should now be able to withdraw her token whenever she wishes.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Rewards/RewardsDistributor.sol#L277-L292 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Rewards/RewardsDistributor.sol#L416-L423

Vulnerability details

Impact

Attacker can drain all rewardTokens from RewardsDistributor contract

Proof of Concept

The claimRewardToken function calls an internal function _grantRewardToken

Here is the claimRewardToken function:

function claimRewardToken(address holder, VToken[] memory vTokens) public {
        uint256 vTokensCount = vTokens.length;

        _ensureMaxLoops(vTokensCount);

        for (uint256 i; i < vTokensCount; ++i) {
            VToken vToken = vTokens[i];
            require(comptroller.isMarketListed(vToken), "market must be listed");
            Exp memory borrowIndex = Exp({ mantissa: vToken.borrowIndex() });
            _updateRewardTokenBorrowIndex(address(vToken), borrowIndex);
            _distributeBorrowerRewardToken(address(vToken), holder, borrowIndex);
            _updateRewardTokenSupplyIndex(address(vToken));
            _distributeSupplierRewardToken(address(vToken), holder);
        }
        rewardTokenAccrued[holder] = _grantRewardToken(holder, rewardTokenAccrued[holder]);
    }

And the _grantRewardToken function:

function _grantRewardToken(address user, uint256 amount) internal returns (uint256) {
        uint256 rewardTokenRemaining = rewardToken.balanceOf(address(this));
        if (amount > 0 && amount <= rewardTokenRemaining) {
            rewardToken.safeTransfer(user, amount);
            return 0;
        }
        return amount;
    }

_grantRewardToken sends amount==rewardTokenAccrued[user] to the recipient address, then returns a value(0 or amount) which will be used to update rewardTokenAccrued[user] in claimRewardToken function.

When the rewardToken is an ERC777 token and recipient is a contract, rewardToken.safeTransfer(user, amount); in _grantRewardToken function, will call a tokensReceived hook in the recipient contract.

The malicious contract creator would have programmed tokensReceived to call claimRewardToken again, and continue the loop until all the rewards has been drained.

This reentrancy is possible because the protocol only updates rewardTokenAccrued, that an address can claim, to 0 after the token has been sent to the address, which allows the malicious contract to recall claimRewardToken before its rewardTokenAccrued is set to 0.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a nonReentrant modifier to the claimRewardToken function:

bool public nonEntered=true;
modifier nonReentrant() {
        require(_notEntered, "re-entered");
        _notEntered = false;
        _;
        _notEntered = true; // get a gas-refund post-Istanbul
    }
...
function claimRewardToken(address holder, VToken[] memory vTokens) public nonReentrant {
    ...
}

Assessed type

Reentrancy