Nibbl contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the buy function of the NibblVault contract, when msg.value > _lowerCurveDiff, the fee for SecondaryCurve part is not charged.

            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);
            } 

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

The fee for the SecondaryCurve part is complex to charge. I implemented the _caculateFeeSecondaryCurve and _reverseFeeSecondaryCurve functions to do the relevant calculations. The _caculateFeeSecondaryCurve function is used to calculate the value after the fee is charged, but not to actually charge the fee. The _reverseFeeSecondaryCurve function is used to calculate the value before the fee is charged.

            if (_lowerCurveDiff >= _caculateFeeSecondaryCurve(msg.value)) {
                _purchaseReturn = _buySecondaryCurve(msg.value, _totalSupply);
            } else {
                //Gas Optimization
                _purchaseReturn = _initialTokenSupply - _totalSupply;
                secondaryReserveBalance += _lowerCurveDiff;
                uint256 _amount = _reverseFeeSecondaryCurve(_lowerCurveDiff);
                _chargeFeeSecondaryCurve(_amount);
                // _purchaseReturn = _buySecondaryCurve(_to, _lowerCurveDiff);
                _purchaseReturn += _buyPrimaryCurve(msg.value - _amount, _totalSupply + _purchaseReturn);
            } 

    function _caculateFeeSecondaryCurve(uint256 _amount) private returns(uint256) {
       address payable _factory = factory;
        uint256 _adminFeeAmt = NibblVaultFactory(_factory).feeAdmin();
        uint256 _feeAdmin = (_amount * _adminFeeAmt) / SCALE ;
        uint256 _feeCurator = (_amount * curatorFee) / SCALE ;
        return _amount - (_feeAdmin + _feeCurator);
    }
    function _reverseFeeSecondaryCurve(uint256 _amount) private returns(uint256) {
       address payable _factory = factory;
        uint256 _adminFeeAmt = NibblVaultFactory(_factory).feeAdmin();
        return _amount * SCALE / (SCALE - (_adminFeeAmt + curatorFee));
    }

Lines of code

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

Vulnerability details

Impact

Basket contracts are deployed using a proxy, but Basket inherits a non-upgradable version of the ERC721 contract, which makes it impossible to set the contract's name and symbol.

    constructor(string memory name_, string memory symbol_) {
        _name = name_;
        _symbol = symbol_;
    }

Proof of Concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L6-L7 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L13-L14 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L44-L47

Tools Used

None

Recommended Mitigation Steps

Using upgradable version of the ERC721 contract https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC721/ERC721Upgradeable.sol

Lines of code

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

Vulnerability details

Impact

In the initiateBuyout function of the NibblVault contract, the second parameter of the BuyoutInitiated event is _buyoutBid instead of _currentValuation, and since the excess Ether in _buyoutBid is transferred to the user, the actual buyout price for the user is the _currentValuation variable. The user can use a large amount of Ether to get a large _buyoutBid variable, however the actual amount of Ether spent by the user is _currentValuation. Events emitted by the smart contract are used off-chain, and incorrect event parameters may have an impact on the user's trading behavior

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

-       emit BuyoutInitiated(msg.sender, _buyoutBid);
+      emit BuyoutInitiated(msg.sender, _currentValuation);