Lines of code

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&gt;

    /// @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&gt;

    /// @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.

---

The text was updated successfully, but these errors were encountered:

👍 1 GalloDaSballo reacted with thumbs up emoji 👀 1 GalloDaSballo reacted with eyes emoji

All reactions

• 👍 1 reaction
• 👀 1 reaction