NFTX contest - janbro's results

Handle

janbro

Vulnerability details

Summary

An attacker can cause an overflow in the flashLoan function where 0 tokens are burned after a large amount of tokens are minted, if there is a flash loan fee, due to not utilizing safe math.

Risk Rating

Critical

Vulnerability Details

An attacker can craft an amount to mint such that the amount + fee overflows to a small value or even 0, which would leave the user with amount tokens minted and only a small portion of the tokens being burned. This would allow the attacker to mint enough tokens to drain the pool of all NFTs. Note: The fee must be larger than the current totalSupply(), however this attack can be utilized when a new vault with a non zero flash fee is created.

ERC20FlashMintUpgradeable.sol, Line 64:

function flashLoan(
    IERC3156FlashBorrowerUpgradeable receiver,
    address token,
    uint256 amount,
    bytes memory data
)
    public virtual override returns (bool)
{
    uint256 fee = flashFee(token, amount);
    _mint(address(receiver), amount);
    require(receiver.onFlashLoan(msg.sender, token, amount, fee, data) == RETURN_VALUE, "ERC20FlashMint: invalid return value");
    uint256 currentAllowance = allowance(address(receiver), address(this));
    require(currentAllowance >= amount + fee, "ERC20FlashMint: allowance does not allow refund");
    _approve(address(receiver), address(this), currentAllowance - amount - fee);
    _burn(address(receiver), amount + fee);
    return true;
}

Impact

Pool drained of NFTs

Proof of Concept

The fee has to be an amount larger than 0, then the amount to loan should be calculated with 115792089237316195423570985008687907853269984665640564039457584007913129639936 - fee.

1. call flashLoan(receiver, token, 115792089237316195423570985008687907853269984665640564039457584007913129639931, data)
2. The fee is calculated as 5
3. _mint(address(receiver), 115792089237316195423570985008687907853269984665640564039457584007913129639931) mints large value of tokens
4. The attacker drains the contract of NFTs in the onFlashLoan function of their malicious contract with their newly minted tokens using redeem(...)
5. require(currentAllowance >= amount + fee, "ERC20FlashMint: allowance does not allow refund"); passes due to the overflow in amount + fee being equivalent to 0. The current allowance can be ignored.
6. _burn(address(receiver), amount + fee) results in 0 tokens being burnt, allowing the attacker to successfully drain the contract of all NFTs and keep any remaining tokens.

Tools Used

Manual Code Review

Recommended Mitigation Steps

Utilize safe math in the flashLoan function when calculating amount + fee

Handle

janbro

Vulnerability details

Summary

_sendForReceiver is vulnerable to reentrancy. This enables a receiver to drain the remaining fees to distribute.

Risk Rating

Critical

Vulnerability Details

NFTXFeeDistributor.sol

Line 163: (bool success, bytes memory returnData) = address(_receiver.receiver).call(payload);

_sendForReceiver can be reentered before remaining funds are distributed to other receivers through the external call. The distribute(...) function on line 48 has no privilege checks and can be subsequently called by a contract that is listed as a receiver.

Impact

Stolen funds

Proof of Concept

Given an amount 1,000x10**18 to distribute, a defaultTreasuryAlloc of 2x10**17, an _receiver.allocPoint of 2x10**18 and 5 receivers. In the best case the receiver is in position 0 of the feeReceivers[vaultId] array. Distribute is called, and the malicious receiver contract receives (1000*10**18) * 2*10**18 / (10*10**18) = 200x10**18. The malicious contract then calls donate() again in their contracts receiveRewards callback function and is awarded (800*10**18-) * 2*10**18 / (10*10**18) = 160x10**18. Although the treasury will take a fee everytime distribute() is called, the attacker can continue to steal funds as long as the balance of the fee distributor is greathr than _treasuryAlloc although the default value in this example is negligible. Subsequent safeTransfer's to other receivers do not revert since the amount sent is calculated based on the current balance of the fund, which allows the attacker to leave no funds in the distributor without worrying about the transaction reverting in subsequent transfers. The following equation can be used to determine how much reward can be taken given n steps:

Tools Used

Manual Code Review

Recommended Mitigation Steps

Add a reentrancy guard to _sendForReceiver to prevent reentrancy due to the external call

Handle

janbro

Vulnerability details

Summary

A malicious receiver can cause another receiver to lose out on distributed fees by returning false for tokensReceived when receiveRewards is called on their receiver contract.

Risk Rating

Medium

Vulnerability Details

A malicious receiver can cause another receiver to lose out on distributed fees by returning false for tokensReceived when receiveRewards is called on their receiver contract. This causes the fee distributor to double spend the amountToSend because the contract incorrectly assumes the returned data is truthful.

NFTXFeeDistributor.sol

Line 163: (bool success, bytes memory returnData) = address(_receiver.receiver).call(payload);

Impact

Any subsequent receivers not receiving funds

Tools Used

Manual Code Review

Recommended Mitigation Steps

Don't trust return data from externally called contracts. Only utilize whether the transaction succeeds to determine if the treasury fallback should be called.

Line 165: if (!success) {

Handle

janbro

Vulnerability details

Summary

The direct redeem fee can be circumvented

Risk Rating

Medium

Vulnerability Details

Since the random NFT is determined in the same transaction a payment or swap is being executed, a malicious actor can revert a transaction if they did not get the NFT they wanted. Combined with utilizing Flashbots miners which do not publish transactions which revert with FlashbotsCheckAndSend, there would be no cost to constantly attempting this every block or after the nonce is updated from getPseudoRand().

NFTXVaultUpgradeable.sol

Line 374: uint256 tokenId = i < specificIds.length
                ? specificIds[i]
                : getRandomTokenIdFromFund();

Impact

The directReedemFee can be avoided and users lose on potential earnings.

Proof of Concept

Transfer ownership of ERC20 tokens to attack contract

function revertIfNotSpecifiedID(uint256 targetTokenID) public {
    NFTXVaultUpgradeable vault = NFTXVaultUpgradeable(_vault);
    uint256[] resultID = vault.redeem(1,[]);
    require(resultID[0] == targetTokenID);
}

Tools Used

Manual Code Review

Recommended Mitigation Steps

Use a commit-reveal pattern for NFT swaps and redemptions