Nibbl contest - 0xNineDec's results

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L389

Vulnerability details

Impact

If the NibblVault.sell function is called with the wrong parameters and no _to address is given, the counterpart of ether that must be sent to the user will be lost, acting the named function as an ether sink.

Proof of Concept

The NibblVault.buy function calls the _mint method which has a built in require statement that prevents having the address(0) as the recipient of the minted tokens.

The scenario is different with the NibblVault.sell. The _burn method does not use the _to parameter which is an input for the further safeTransferETH(_to, _saleReturn) call, in the end of the function in question, which performs a low level ether transfer.

In every supply state of the token, value can be irreversibly lost. There is no safe case between the scenario where the sell amount estimation is made via _sellPrimaryCurve or _sellSecondaryCurve because the return of either functions is the forwarded effective ETH value, having the _to parameter constant for each call.

The following scenarios may happen:

Scenario A:

• Alice wants to call NibblVault.sell.
• The third parameter of the mentioned function by mistake is the address(0)
• Alice has lost every token and received no ether in exchange.

Scenario B:

Based on the given testing beta, the swap is performed automatically and no input prompt is given to the user to specify the _to parameter while calling NibblVault.sell. The ether counterpart is automatically sent to the msg.sender. This means that a development error of the Web3 application which misses the third parameter may also convey in a lost of funds.

Both scenarios (primary and secondary curve) were tested with two scripts on the same test framework given by the development team, the return values are console logged:

it("Will lose ether on primary curve", async function () {
        const _buyAmount = ethers.utils.parseEther("1");
        const _feeTotal = constants.FEE_ADMIN.add(constants.FEE_CURATOR).add(constants.FEE_CURVE);
        const _initialSecondaryBalance = await vaultContract.secondaryReserveBalance();
        const _initialPrimaryBalance = await vaultContract.primaryReserveBalance();
        const _buyAmountWithFee = _buyAmount.sub(_buyAmount.mul(_feeTotal).div(constants.SCALE));
        const _purchaseReturn = await mintTokens(testBancorFormula, constants.initialTokenSupply, constants.initialPrimaryReserveBalance, constants.primaryReserveRatio, _buyAmountWithFee);
        let _initialBalanceFactory = await admin.provider.getBalance(vaultFactoryContract.address);
        let _newSecBalance = _initialSecondaryBalance.add((_buyAmount.mul(constants.FEE_CURVE)).div(constants.SCALE));
        await vaultContract.connect(buyer1).buy(_purchaseReturn, await buyer1.getAddress(), { value: _buyAmount });
    
        // ------------------Tokens Bought----------------
        // Sell Tokens
        const _feeAccruedInitial = await vaultContract.feeAccruedCurator();
        const _sellAmount = _purchaseReturn.div(2); //Only selling half the amount bought
        const _sellReturn = await burnTokens(testBancorFormula, constants.initialTokenSupply.add(_purchaseReturn),  _initialPrimaryBalance.add(_buyAmountWithFee), constants.primaryReserveRatio, _sellAmount);
        const _sellReturnWithFee = _sellReturn.sub(_sellReturn.mul(_feeTotal).div(constants.SCALE));
        _initialBalanceFactory = await admin.provider.getBalance(vaultFactoryContract.address);

        console.log(`Initial Address(0) ETH: ${ethers.utils.formatEther(await ethers.provider.getBalance(ethers.constants.AddressZero))}`);
        console.log(`Initial Seller tokens: ${ethers.utils.formatEther(await vaultContract.balanceOf(buyer1.getAddress()))}`);

        await vaultContract.connect(buyer1).sell(_sellAmount, _sellReturnWithFee, ethers.constants.AddressZero);
        
        console.log("---")
        console.log(`Final Address(0) ETH: ${ethers.utils.formatEther(await ethers.provider.getBalance(ethers.constants.AddressZero))}`);
        console.log(`Final Seller tokens: ${ethers.utils.formatEther(await vaultContract.balanceOf(buyer1.getAddress()))}`);

})

    it("Will lose ether on secondary curve", async function () {
    const _sellAmount = (constants.initialTokenSupply).div(5);
    let _balanceAddr1 = await addr1.provider.getBalance(await addr1.getAddress());
    const _expectedSaleReturn = await burnTokens(testBancorFormula, constants.initialTokenSupply, constants.initialSecondaryReserveBalance, constants.initialSecondaryReserveRatio, _sellAmount);        
    const _expectedSaleReturnWithFee = _expectedSaleReturn.sub(_expectedSaleReturn.mul(constants.FEE_SECONDARY_CURVE).div(constants.SCALE));
   
    console.log(`Initial Address(0) ETH: ${ethers.utils.formatEther(await ethers.provider.getBalance(ethers.constants.AddressZero))}`);
    console.log(`Initial Seller tokens: ${ethers.utils.formatEther(await vaultContract.balanceOf(curator.getAddress()))}`);

    await vaultContract.connect(curator).sell(_sellAmount, _expectedSaleReturnWithFee, ethers.constants.AddressZero);
    console.log("---")
    console.log(`Final Address(0) ETH: ${ethers.utils.formatEther(await ethers.provider.getBalance(ethers.constants.AddressZero))}`);
    console.log(`Final Seller tokens: ${ethers.utils.formatEther(await vaultContract.balanceOf(curator.getAddress()))}`);
})

The results given by both simulations are the following:

On both scenarios the address(0) increased its ether balance and also the token balance of the user decreased.

This issue has a chance to happen every time an user wants to call NibblVault.sell for every vault generated (one for each fractionalized NFT) which yields in an extremely high likelihood (the probability is exponential because every NFT has its own contract with the same implementation and an user can interact with several fractionalized NFTs). Regarding the impact, each successfully sent transaction with the _to parameter as the address(0) will irreversibly lose funds. Although this is not a properly executed attack, the combination of likelihood of happening and its potential impact convey this to be a considerable vulnerability. The given arguments and final risk level determination are subject to change by the criteria of the judges.

Even if both scenarios (A, B) are not attacks because of having a straightforward non-conflictive solution to this problem, it is important to prevent having further issues with how users interact with the core functions of each generated NFT vault smart contract.

Recommended Mitigation Steps

A non-conflictive mitigation can be done in order to reduce the likelihood of failure on off-chain implementations (such as frontend development or user interaction).

The only internal call of NibblVault.safeTransferETH which parameters can be manipulated is by executing NibblVault.sell.

There are two possible patches to this issue:

• Checking on NibblVault.safeTransferETH(_to, _amount) that _to is not the address(0).
• Checking while calling NibblVault.sell(_amtIn, _minAmtOut, _to) that _to is not the address(0). This option leaves open the chance to send ether to the address(0) if NibblVault.safeTransferETH(_to, _amount) is called with _to as the address(0).