Nibbl contest - zzzitron's results

Lines of code

https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVault.sol#L319-L321

Vulnerability details

Impact

One can potentially buy vault tokens with paying less fees.

Proof of Concept

This diff file to the test/vaultTrade.test.ts compares fees for two cases.

• setup: the current supply is below the initial supply
• case 1) buying almost to the initial supply but within the secondary curve
• case 2) buying past the initial supply The amount to buying difference for these two cases in the example test is 1000 wei. However, the admin fee and curator fee charged for case 1 is higher than the case 2, even though case 2 has purchased a little bit more than the case 1.

Tools Used

hardhat test

Recommended Mitigation Steps

The below snippet is the buy code when token's supply is less then the initial token supply. Instead of accounting in the buy function, one could use the _buySecondaryCurve to do the accounting and charge fees. If the _buySecondaryCurve should be used, however, one cannot just pass _lowerCurveDiff as the argument, because the charge will be subtracted from the passed amount, which will result to buy less than the intended amount. It will lead to missing the fee amount in the secondaryReserveBalance. So, one should add the fee to be charged on top of the _lowerCurveDiff. Additional consideration is the if statement should also consider the fee. Or alternatively, calculate how much token will be bought for the msg.value, and compare that to the (initial supply - total supply). It is also good to add tests for some edge cases to make sure they do not revert. For example, when the msg.value is slightly under/over _lowerCrveDiff, etc.

///// NibblVault.sol:315-323
            if (_lowerCurveDiff >= msg.value) {
                _purchaseReturn = _buySecondaryCurve(msg.value, _totalSupply);
            } else {
                //Gas Optimization
                _purchaseReturn = _initialTokenSupply - _totalSupply;
                secondaryReserveBalance += _lowerCurveDiff;
                // _purchaseReturn = _buySecondaryCurve(_to, _lowerCurveDiff);
                _purchaseReturn += _buyPrimaryCurve(msg.value - _lowerCurveDiff, _totalSupply + _purchaseReturn);
            } 

Nibbl QA Report

Summary/Impression

• It was enjoyable to work the code, because it was well documented with README and comments in the code. The documentation explained the intention and assumption of the code, which makes it easy to read and understand. Also the test code helped to understand the usage of the contracts and write custom tests.
• One might improve the readability by using helper functions.
• One concern for upgradeability is to keep the storage structure through out the update. It is easy to make a mistake with the inheritance structure, which will change the storage structure.

Low

L0: Cast to uint32 does not check overflow

• github link

        uint32 _secondaryReserveRatio = uint32((msg.value * SCALE * 1e18) / (_initialTokenSupply * _initialTokenPrice));

The casting to uint32 does not check if the value to cast is truncated. For some reason very small number for the _initialTokenSupply or _initialTokenPrice is given, it might go over the maximum value of the uint32. In that case, the curator will get very small share for the bigger amount of the value sent.

L1: possible overflow

• github link

        twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative

Although, it is not likely to happen since the buyout rejection period is short (i.e. 5 days), the vumulativeValuation might overflow if the rejection period should be changed to be longer. It is preventative to add unchecked block, so it will work no matter how long the rejection period might be. It will also save some gas. If it should be changed, Keep in mind also to add another unchecked block to line40

L2: Usage of transfer

• github link

        _to.transfer(address(this).balance);

The usage of transfer to send ETH is not anymore advised, due to its limited gas stipend. about transfer

L3: Lack of zero address checks

There are multiple cases with lacking zero address checks.

L3-0: basket implementation address in ProxyBasket

• ProxyBasket.sol:20 The address for the implementation contract is not checked in ProxyBasket nor in NibbleValutFactory upon updating implementation. Since deployed ProxyBasket cannot change the implementation address, it is safer to check the address.

L3-1: safeTransferETH to zero address in NibblVault

• NibblVault.sol:569 The safeTransferETH will transfer to zero address. This is used through out in the code, also to transfer ETH for selling and redeeming as well. If the user should make a mistake to input zero address, the fund will be lost.

L3-2: feeTo address in NibblVaultFactory

• NibblVaultFactory.sol:132 If the feeTo address is set to zero, the withdrawAdminFee will send all the balance to the address.

L3-3: curator address in NibblVault

• NibbleVault.sol:487 If the curator is set to zero address, the curatorFee will be locked in the vault, since nobody can update the curator address anymore.

L3-4: addresses in the constructor of the NibblVaultFactory

• NibbleVaultFactory.sol:24-26 The initial addresses for vaultImplementation, feeTo, basketImplementation can be set to zero.

Non-critical

N0: Fixed values can be immutable in NibbleVault

Some values are meant to be fixed throughout the code, for example:

• initialTokenSupply
• initialTokenPrice
• fictitiousPrimaryReserveBalance Those values can be set to be immutable, so that they will be never accidentally changed. It also makes it easy to understand the code, as well as saving gas.

N1: Mismatching from the documentation to the code

N1-0: Fixed primaryReserveRatio in NibblVault

• NibbleVault.sol:28 According to the documentation:

Also, note that the curator would want the reserve ratio of the primary curve to be a decent amount (50% in this example) so that his share of ownership doesn't get diluted too much

However, the primaryReserveRatio is hard-coded to be 20% and curator cannot set it.

N1-1: Curve Fee to the collector

According to the README:

All subsequent trading fees goes to the original artist/collector

The curve fee is stop charged after the secondary curve matches the primary curve.

• github link