Nibbl contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/Twav/Twav.sol#L40

Vulnerability details

Impact

Twav._getTwav() might revert when it should work properly. In this case, the main functions like NibblVault.buy() and NibblVault.sell() will revert also.

Proof of Concept

According to the logic, Twav.lastBlockTimeStamp saves a modularized timestamp so current timestamp might be smaller than the previous one. So (_twavObservationCurrent.timestamp - _twavObservationPrev.timestamp) might meet the underflow error.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We can use unchecked calculation for the above calculation. We can modify L40 like below.

uint32 _timeElapsed; 
unchecked {
    _timeElapsed = _twavObservationCurrent.timestamp - _twavObservationPrev.timestamp;
}
_twav = (_twavObservationCurrent.cumulativeValuation - _twavObservationPrev.cumulativeValuation) / _timeElapsed;

Lines of code

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

Vulnerability details

Impact

The "if" condition of Twav._getTwav() is missing some edge cases. In this case, this function will return 0 which is different from the correct value and it will affect the main functions like NibblVault.buy() and NibblVault.sell().

Proof of Concept

I think this condition is to confirm at least 4 values were saved for twav calculation. Btw this timestamp would be zero even though there are more than 4 values properly as it's modularized by 2**32. In this case, the if condition will be false and this function will return 0.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I see "cumulativeValuation" is increasing all the time and recommend replacing "timestamp" with "cumulativeValuation".

if (twavObservations[TWAV_BLOCK_NUMBERS - 1].cumulativeValuation != 0) {

Summary

I've found 3 row-risk issues and some non-critical issues.

Low-Risk Issues

1. Wrong comments

• require(buyoutEndTime <= block.timestamp, "NibblVault: buyoutEndTime <= now"); It must be "NibblVault: buyoutEndTime > now".

• require(block.timestamp >= minBuyoutTime, "NibblVault: minBuyoutTime < now"); It must be "NibblVault: minBuyoutTime > now".

2. Use safeTransfer(), safeTransferFrom() consistently instead of transfer(), transferFrom(). This is the same previous issue as a reference. L-01

• IERC20(_asset).transfer(_to, IERC20(_asset).balanceOf(address(this)));

• IERC20(_assets[i]).transfer(_to, IERC20(_assets[i]).balanceOf(address(this)));

• IERC20(_token).transfer(msg.sender, IERC20(_token).balanceOf(address(this)));

• IERC20(_tokens[i]).transfer(msg.sender, IERC20(_tokens[i]).balanceOf(address(this)));

3. Missing events for admin functions that change important parameters

• NibblVaultFactory.proposeNewAdminFee()

Non-critical Issues

1. require()/revert() statements should have descriptive reason strings

• require(_success);

2. Natspec incomplete

• withdrawERC721() @param _to

• withdrawERC721Unsafe() @param _to

• withdrawERC1155() @param _to

• _buyPrimaryCurve @param _totalSupply

• _buySecondaryCurve @param _totalSupply

• _sellPrimaryCurve @param _totalSupply

• _sellSecondaryCurve @param _totalSupply

• redeem @param to

3. Needless ()

• (primaryReserveBalance)
• (_initialTokenSupply + _amtIn)
• (_buyoutBid - _currentValuation)
• (twavObservationsIndex + TWAV_BLOCK_NUMBERS)
• (twavObservationsIndex + TWAV_BLOCK_NUMBERS)
• (_index)

1. Use != 0 instead of > 0 for uint variables

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

2. No need to initialize variables with default values

• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L70
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L93
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L506
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L525
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L547

3. Use ++i instead of i++, i+=1, also unchecked increments in for-loops will save gas cost

• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L70
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L93
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L506
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L525
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L547

4. An array’s length should be cached to save gas in for-loops

• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L43
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L70
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L93
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L506
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L525
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L547

5. Non-strict inequalities are cheaper than strict ones

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

6. Usage of unchecked can reduce the gas cost

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

7. require()/revert() strings longer than 32 bytes cost extra gas

• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L48
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L49
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L105
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L131
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L141
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L149
• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L166

8. Check zero amount before transfer

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

9. Check "_feeAdmin != 0" instead of "_adminFeeAmt > 0". The real transfer amount is _feeAdmin and this value might be zero even though _adminFeeAmt > 0 according to fee calculation

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

10. Needless calculation From the calculation, we can see "(_index + 1) % TWAV_BLOCK_NUMBERS" is same as "twavObservationsIndex" because "_index" is the previous index of "twavObservationsIndex".

• https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Twav/Twav.sol#L39