Nibbl contest - shenwilly's results

Low Risk Vulnerabilities

1. getBasketAddress could return inaccurate value if basketImplementation changes

It is possible that the basketImplementation gets upgraded to a new address. If an integrator relies on getBasketAddress to deterministically determine the address of a basket that has been deployed using the old implementation, it will return the incorrect address.

Recommended Mitigation Steps

Consider adding an extra parameter _implementation or overload the function with the extra parameter:

function getBasketAddress(address _curator, string memory _mix, address _basketImplementation) public override view returns(address _basket) {
    bytes32 newsalt = keccak256(abi.encodePacked(_curator, _mix));
    bytes memory code = abi.encodePacked(type(ProxyBasket).creationCode, uint256(uint160(_basketImplementation)));
    bytes32 hash = keccak256(abi.encodePacked(bytes1(0xff), address(this), newsalt, keccak256(code)));
    _basket = address(uint160(uint256(hash)));     
}

Related Codes

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L88-L93

2. Missing sanity check on _minBuyoutTime

When creating a vault, a faulty deployment payload could set minBuyoutTime to a very high value, virtually disabling the buyout function.

Recommended Mitigation Steps

Consider adding a sanity check to make sure it's set within a reasonable value:

require(_minBuyoutTime <= 52 weeks, "NibblVault: minBuyoutTime must not be longer than one year");

Related Codes

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

3. Check _feeAdmin instead of _adminFeeAmt

When charging fee in _chargeFee and _chargeFeeSecondaryCurve, there's a check to ensure _adminFeeAmt is larger than zero before transferring the funds to feeAdmin. It is possible for _adminFeeAmt to be higher than zero yet _feeAdmin returns zero because of rounding. Therefore, it will be more accurate to use _feeAdmin instead.

PoC

uint256 _feeAdmin = (_amount * _adminFeeAmt) / SCALE ;

When _amount * _adminFeeAmt is lower than SCALE, _feeAdmin will have the value of 0.

Recommended Mitigation Steps

Check that _feeAdmin is larger than 0 instead:

if(_feeAdmin > 0) {
    safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract
}

Related Codes

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

Non-Critical Vulnerabilities

1. Uncustomizable Basket details

When creating a basket, the basket name and symbol is fixed to "NFT Basket" and "NFTB" respectively. It would be hard for users to distinguish one basket from another when seen through etherscan or wallets.

Recommended Mitigation Steps

Consider adding an extra parameter so basket creator can append a unique identifier for the baskets. For instance, when making a basket containing CryptoPunks, the name and symbol would be:

NFT Basket - Punks
NFTB-PUNKS

Related Codes

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L82 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L13