User Could Change The State Of The System While In `Pause` Mode

[code423n4]

Lines of code

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

Vulnerability details

Proof-of-Concept

Calling NibblVault.updateTWAP function will change the state of the system. It will cause the TWAP to be updated and buyout to be rejected in certain condition.

When the system is in Pause mode, the system state should be frozen. However, it was possible someone to call the NibblVault.updateTWAP function during the Pause mode, thus making changes to the system state.

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

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

Recommended Mitigation Steps

Ensure that the NibblVault.updateVault function cannot be called when the system is in Pause mode.

Add the whenNotPaused modifier to the function.

/// @notice Updates the TWAV when in buyout
/// @dev TWAV can be updated only in buyout state
function updateTWAV() external override whenNotPaused {
    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
    }
}

[mundhrakeshav]

56

[HardlyDifficult]

56

It's not clear to me how this is a dupe of #56

This is a valid concern and potentially a change worth making.

It will cause the TWAP to be updated and buyout to be rejected

This makes me think Medium risk is correct here. In this scenario a buyout could be rejected without allowing other users to challenge that -- seemingly breaking one of the benefits behind using Twap for this logic.