Nibbl contest - hyh's results

Lines of code

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

Vulnerability details

A malicious user can run updateTWAV() on each block, quickly replacing all four values of the twavObservations array with the most recent valuation. I.e. the time weighted averaging essence of the recorded price can be directly reduced to always be just most recent price by any actor who can extract profit from such an update.

I.e. when there are no one around with any interests in price manipulation, TWAP array will hold an average over some random last period as TWAP is updated on user operations. Once there be an actor interested for the price to be manipulated, the price cease to being TWAP as the twavObservations array will be filled with the most recent entry, so the price becomes simply the spot one, which can be changed quickly to extract a profit.

Setting the severity to be high as that's a violation of system logic of price smoothing, which can lead to altering of the system's behavior, say last minute buyout. I.e. a whale can buy some Vault tokens, wait for the very end of buyout price discovery period, quickly sell his tokens and force price update by repeating updateTWAV() call, winning the bid as the reduced price allows the buyout. Token holders will not have the time to react in this case, i.e. no real price discovery will take place.

Proof of Concept

updateTWAV() can be called by anyone and only requires different blocks to proceed:

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

    /// @notice Updates the TWAV when in buyout
    /// @dev TWAV can be updated only in buyout state
    function updateTWAV() external override {
        require(status == Status.buyout, "NibblVault: Status!=Buyout");
        uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
        if (_blockTimestamp != lastBlockTimeStamp) {
            _updateTWAV(getCurrentValuation(), _blockTimestamp);   
            _rejectBuyout(); //For the case when TWAV goes up when updated externally
        }
    }

It isn’t the main issue as other operations can be used for forced price update if the access to updateTWAV() be restricted. It, however, add some convenience to the attack.

The core issue is that TWAV array update happens unconditionally, always replacing the oldest value, i.e. there are no mechanics to prevent price weighting to be reduced to become non-existent:

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

    /// @notice updates twavObservations array
    /// @param _blockTimestamp timestamp of the block
    /// @param _valuation current valuation
    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;
    }

Recommended Mitigation Steps

Consider introducing a delay before adding the entry to TWAV array, say require that MIN_TWAV_LENGTH of time have to be in the array.

For example, if last entry removal on price update causes (earliest - oldest) time to drop below MIN_TWAV_LENGTH then refuse the update. This will enforce the averaging at least over MIN_TWAV_LENGTH period, which should be big enough, say 2-3 days, so token holders have the necessary time to act on the valuation change.

1. ecrecover returning zero for non-matching signature isn't controlled (low)

ecrecover should be checked to be non-zero as zero return value means invalid signature:

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

        address signer = ecrecover(toTypedMessageHash(structHash), v, r, s);
        require(signer == owner, "NibblVault: invalid signature");
        _approve(owner, spender, value);
    }

Recommended Mitigation Steps

Consider adding, for example, require(owner != address(0), "owner cannot be zero") to NibblVault's permit().

Even if _approve(0x0, ... doesn't seem to cause much issues, the ability to run it is a malfunction.

2. Floating pragma is left in AccessControlMechanism (non-critical)

As different compiler versions have critical behavior specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced

Proof of Concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/AccessControlMechanism.sol#L4

pragma solidity ^0.8.0;

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Interfaces/IAccessControlMechanism.sol#L4

pragma solidity ^0.8.0;

Recommended Mitigation Steps

Consider fixing AccessControlMechanism's version to 0.8.10

3. Basket's and NibblVault's withdrawMultipleERC1155 don't check argument array length before accessing (non-critical)

On calling with arrays of different lengths various malfunctions are possible as the arrays are used as given. System then will fail with low level array access message.

Proof of Concept

Array lengths aren't controlled:

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

    function withdrawMultipleERC1155(address[] memory _tokens, uint256[] memory _tokenIds, address _to) external override {
        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
        for (uint256 i = 0; i < _tokens.length; i++) {

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

    function withdrawMultipleERC1155(address[] memory _assets, uint256[] memory _assetIDs, address _to) external override boughtOut {
        require(msg.sender == bidder, "NibblVault: Only winner");
        for (uint256 i = 0; i < _assets.length; i++) {
            uint256 balance = IERC1155(_assets[i]).balanceOf(address(this),  _assetIDs[i]);

Recommended Mitigation Steps

Consider requiring lengths of _tokens and _tokenIds arrays to match.