Nibbl contest - WatchPug's results

Lines of code

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

Vulnerability details

function _getTwav() internal view returns(uint256 _twav){
    if (twavObservations[TWAV_BLOCK_NUMBERS - 1].timestamp != 0) {
        uint8 _index = ((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;
        TwavObservation memory _twavObservationCurrent = twavObservations[(_index)];
        TwavObservation memory _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];
        _twav = (_twavObservationCurrent.cumulativeValuation - _twavObservationPrev.cumulativeValuation) / (_twavObservationCurrent.timestamp - _twavObservationPrev.timestamp);
    }
}

Since _blockTimestamp is uint32, subtraction underflow is desired at _twavObservationCurrent.timestamp - _twavObservationPrev.timestamp.

See: https://github.com/Uniswap/v2-periphery/blob/master/contracts/examples/ExampleOracleSimple.sol#L43

function update() external {
    (uint price0Cumulative, uint price1Cumulative, uint32 blockTimestamp) =
        UniswapV2OracleLibrary.currentCumulativePrices(address(pair));
    uint32 timeElapsed = blockTimestamp - blockTimestampLast; // overflow is desired

Because the solidity version used by the current implementation is 0.8.10, and there are some breaking changes in Solidity v0.8.0:

Arithmetic operations revert on underflow and overflow.

Ref: https://docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html#silent-changes-of-the-semantics

The timestamp subtraction may revert due to underflow.

Impact

Since _getTwav() is used in NibblVault.sol#_rejectBuyout(), if it reverts and there is a buyout, an essential feature of the NibblVault contract will be unavailable, causing users' funds to be frozen in the contract.

Recommendation

Change to:

function _updateTWAV(uint256 _valuation, uint32 _blockTimestamp) internal {
    uint32 _timeElapsed; 
    unchecked {
        _timeElapsed = _blockTimestamp - lastBlockTimeStamp;
    }

    uint256 _prevCumulativeValuation = twavObservations[((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS].cumulativeValuation;
    unchecked {
        twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative
    }
    
    twavObservationsIndex = (twavObservationsIndex + 1) % TWAV_BLOCK_NUMBERS;
    lastBlockTimeStamp = _blockTimestamp;
}

function _getTwav() internal view returns(uint256 _twav){
    if (twavObservations[TWAV_BLOCK_NUMBERS - 1].timestamp != 0) {
        uint8 _index = ((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS;
        TwavObservation memory _twavObservationCurrent = twavObservations[(_index)];
        TwavObservation memory _twavObservationPrev = twavObservations[(_index + 1) % TWAV_BLOCK_NUMBERS];
        unchecked {
            _twav = (_twavObservationCurrent.cumulativeValuation - _twavObservationPrev.cumulativeValuation) / (_twavObservationCurrent.timestamp - _twavObservationPrev.timestamp);
        }
    }
}

Lines of code

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

Vulnerability details

function buy(uint256 _minAmtOut, address _to) external override payable notBoughtOut lock whenNotPaused returns(uint256 _purchaseReturn) {
    //Make update on the first tx of the block
    if (status == Status.buyout) {
        uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
        if (_blockTimestamp != lastBlockTimeStamp) {
            _updateTWAV(getCurrentValuation(), _blockTimestamp);   
            _rejectBuyout();
        }
    }
    uint256 _initialTokenSupply = initialTokenSupply;
    uint256 _totalSupply = totalSupply();
    if (_totalSupply >= _initialTokenSupply) {
        _purchaseReturn = _buyPrimaryCurve(msg.value, _totalSupply);
    } else {
        uint256 _lowerCurveDiff = getMaxSecondaryCurveBalance() - secondaryReserveBalance;
        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);
        } 
    }
    require(_minAmtOut <= _purchaseReturn, "NibblVault: Return too low");
    _mint(_to, _purchaseReturn);
    emit Buy(msg.sender, _purchaseReturn, msg.value);
}

When the price is in the SecondaryCurve, and the user can buy with msg.value == getMaxSecondaryCurveBalance() - secondaryReserveBalance + 1 to escape the CuratorFee and AdminFee for the part in the SecondaryCurve of the purchase. See L320, _initialTokenSupply - _totalSupply is the part that escaped the fees.

It appears that this is caused by an incorrect code update intended for Gas Optimization. A very similar code in the sell() function was updated correctly: https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L373-L386

Impact

The issue will cause the protocol and the curators to lose part of their fees.

Recommendation

Change to:

if (_totalSupply >= _initialTokenSupply) {
    _purchaseReturn = _buyPrimaryCurve(msg.value, _totalSupply);
} else {
    uint256 _lowerCurveDiff = getMaxSecondaryCurveBalance() - secondaryReserveBalance;
    if (_lowerCurveDiff >= msg.value) {
        _purchaseReturn = _buySecondaryCurve(msg.value, _totalSupply);
    } else {
        //Gas Optimization
        _purchaseReturn = _initialTokenSupply - _totalSupply;
        secondaryReserveBalance += _lowerCurveDiff;
        _purchaseReturn = _chargeFee(_purchaseReturn);
        // _purchaseReturn = _buySecondaryCurve(_lowerCurveDiff, _totalSupply);
        _purchaseReturn += _buyPrimaryCurve(msg.value - _lowerCurveDiff, _totalSupply + _purchaseReturn);
    } 
}

Lines of code

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

Vulnerability details

function _updateTWAV(uint256 _valuation, uint32 _blockTimestamp) internal {
    uint32 _timeElapsed; 
    unchecked {
        _timeElapsed = _blockTimestamp - lastBlockTimeStamp;
    }

    uint256 _prevCumulativeValuation = twavObservations[((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS].cumulativeValuation;
    twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative
    twavObservationsIndex = (twavObservationsIndex + 1) % TWAV_BLOCK_NUMBERS;
    lastBlockTimeStamp = _blockTimestamp;
}

Because the solidity version used by the current implementation is 0.8.10, and there are some breaking changes in Solidity v0.8.0:

Arithmetic operations revert on underflow and overflow.

Ref: https://docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html#silent-changes-of-the-semantics

When _prevCumulativeValuation is big enough, _updateTWAV() will revert due to overflow.

Impact

Since _updateTWAV() is used in NibblVault.sol#buy() and NibblVault.sol#sell(), when it reverts due to overflow, the NibblVault contract as a whole will malfunction, and users' funds may be frozen in the contract.

Recommendation

Change to:

function _updateTWAV(uint256 _valuation, uint32 _blockTimestamp) internal {
    uint32 _timeElapsed; 
    unchecked {
        _timeElapsed = _blockTimestamp - lastBlockTimeStamp;
    }

    uint256 _prevCumulativeValuation = twavObservations[((twavObservationsIndex + TWAV_BLOCK_NUMBERS) - 1) % TWAV_BLOCK_NUMBERS].cumulativeValuation;
    unchecked {
        twavObservations[twavObservationsIndex] = TwavObservation(_blockTimestamp, _prevCumulativeValuation + (_valuation * _timeElapsed)); //add the previous observation to make it cumulative
    }
    
    twavObservationsIndex = (twavObservationsIndex + 1) % TWAV_BLOCK_NUMBERS;
    lastBlockTimeStamp = _blockTimestamp;
}