search

code-423n4 / 2022-06-nibbl-findings

1 stars 0 forks source link

Buyout Rejection Feature Is Not Deterministic #193

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Vulnerability Details

It was observed that the buyout rejection feature is not deterministic and does not always work as intented. The outcome of the buyout rejection depends on a number of external factors and actors, apart from the buyoutRejectionValuation and current valuation.

Per the specification, buyoutRejectionValuation is calculated as buyoutBid * (1 + buyoutRejectionPremium). If the time weighted average valuation (TWAV) becomes greater than or equal to buyoutRejectionValuation within 5 days, the buyout will be rejected. However, due to the current implementation of the TWAV, the buyout rejection does not always work as intented even though the current valuation is greater than or equal to buyoutRejectionValuation , because it depends on many external factors such as how often the _updateTwav function is triggered or when the _updateTwav is triggered.

Proof-of-Concept

Assume that the buyout price is 100 ETH, and the buyoutRejectionValuation is 120 ETH, and the BUYOUT_DURATION = 5 days.

Charles has initiated a buyout by calling the initiateBuyout function. When the initiateBuyout function is triggered, the _updateTWAV will be called and the current valuation will be added to the first position (= zero) of the twavObservations array. The second, third, and forth positions of the twavObservations array are empty and uninitialised at the moment. Following is the visualisation of the twavObservation array at this point:

twavObservation = [CumulativeValuationAtTime0, EMPTY, EMPTY, EMPTY]

Another important point to note is that the getTwav will returns 0 until twavObservations array is filled up or full.

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

function _getTwav() internal view returns(uint256 _twav){
        // @audit following will always return 0 unless the array is full.
    if (twavObservations[TWAV_BLOCK_NUMBERS - 1].timestamp != 0) {
        ..SNIP..
    }
}

The following show two scenarios where the buyout rejection does not work as intented:

Scenario #1

  1. Alice wants to reject Charles's buyout by calling the buy() function. Thus, she brought large number of factionalized tokens from the vault to push the curent valuation to 150 ETH on Day 0 shortly after the buyout is initiated by Charles. Following is the visualisation of the twavObservation array at this point:

    twavObservation = [CumulativeValuationAtTime0, CumulativeValuationAtTime1, EMPTY, EMPTY]

  2. Assume that from this point onwards until the end of the buyout period at Day 5, no one buy and sell any fractionalized token or call the Nibbl.updateTWAV() function. As a result, the twavObservation array remains the same.

  3. At the end of the buyout period at Day 5, Charles successfully buyout the vault/NFT since he was not rejected by the system.

Even though the current valuation is 150 ETH, which is higher than the buyoutRejectionValuation (120 ETH), Charles still managed to buyout the NFT/vault. Per the specification, Charles's buyout should be rejected.

Scenario #2

  1. Alice wants to reject Charles's buyout by calling the buy() function. Thus, she brought large number of factionalized tokens from the vault to push the curent valuation to 150 ETH on Day 0 shortly after the buyout is initiated by Charles. Following is the visualisation of the twavObservation array at this point:

    twavObservation = [CumulativeValuationAtTime0, CumulativeValuationAtTime1, EMPTY, EMPTY]

  2. Someone buys some factionalized tokens by calling the buy() function and the _updateTWAV is triggered. Therefore, the current valuation is added to the 3rd position of the twavObservation array. Following is the visualisation of the twavObservation array at this point:

    twavObservation = [CumulativeValuationAtTime0, CumulativeValuationAtTime1, CumulativeValuationAtTime2, EMPTY]

    Next, _rejectBuyout() function will be triggered. However, since the twavObservations array is not filled up yet, it will return zero. Thus, the buyout is not rejected.

  3. Assume that from this point onwards until the end of the buyout period at Day 5, no one buy and sell any fractionalized token or call the Nibbl.updateTWAV() function. As a result, the twavObservation array remains the same.

  4. At the end of the buyout period at Day 5, Charles successfully buyout the vault/NFT since he was not rejected by the system

Even though the current valuation is 150 ETH, which is higher than the buyoutRejectionValuation (120 ETH), Charles still managed to buyout the NFT/vault. Per the specification, Charles's buyout should be rejected.

Remarks About Existing TWAP Implementation

There is a seperate issue about the existing TWAP implementation, thus refer to my another report for detailed write-up.

In summary, the report highlighted that the current TWAP implementation is not working as intended. If the TWAP is properly implemented and the TWAP's windows/time period is 1 hour, the current valuation of 150 ETH will be returned by the TWAP shortly after 1 hour after Alice has initiated the buy order. Even if there is no transaction happening between Day 0 and Day 5 after Alice's last transaction, the TWAP will still return 150 ETH on Day 5 and cause the buyout to reject. As such, the buyout would not succeed. However, it is not the case in the current implementation.

Impact

  1. The core feature of the protocol does not work as intented under certain scenarios especially when there is no or low user activities.
  2. Loss of Fund for the users. In the above, example, Alice will make a loss because she will be getting back money at the buyout bid valuation instead of the valuation she bought at. Refer to https://github.com/NibblNFT/nibbl-smartcontracts#2-buyout-rejection

Recommended Mitigation Steps

There are a number of design issues that cause this issue:

  1. The twavObservation array depends on user activities to be filled up or populated. If there is no user activities at all, the twavObservation array will be stuck in the same state. The protocol optimistically assumes that there will always be sufficient user activities (users calling buy and sell) to fill up the twavObservation array and keep the TWAP up-to-date.
  2. The twavObservation array is only initialised and being utilised after the buyout has been triggered. Additionally, it needs to wait for at least four (4) valuations to fill up the twavObservations array before the getTwav starts to return the valuation. Thus, it introduces some latency.
  3. The getTwav will returns zero (0) until twavObservations array is filled up or full. However, note that there is a chance that there is no user activities to fill up the twavObservations array. In this case, getTwav will always return zero (0).
  4. The TWAP implementation does not return the average value of a security over a specified time (e.g. 15 minutes, 1 hour, 24 hours). Instead, the current implementation is the average value of a security over a specific number of observations (in this case 4 observations), thus it can be considered as observations weighted average valuation (OWAV). There is a fundamental difference between TWAV and OWAV. Refer to my another report for more details regarding this issue.

Consider the following measures the mitigate the issue:

  1. Implement a proper TWAP. Consider referencing the popular Uniswap V2 TWAP design (https://docs.uniswap.org/protocol/V2/concepts/core-concepts/oracles). If the TWAP is working as intended, in above scenario #1 and #2, at the end of the buyout period, the TWAV value should be 150 ETH, and the buyout will be rejected eventually.
  2. The TWAP mechanism should keep track of the current valuation whenever someone perform a buy or sell even if the vault is not in buyout state. In short, track or update the TWAP all the time whenever there is a state change to the current valuation
  3. Do not simply check whether the buyoutTime has passed to determine if the buyout is successful. The system should check if the current valuation is higher than the buyoutRejectionValuation at the end of the buyout period before deciding if the buyout is successful or not.
sseefried commented 2 years ago

Similar to my issue (#144) but goes into more detail. (Nice work xiaming90!)

HardlyDifficult commented 2 years ago

Great detail. In the end this appears to be the intended design - the concern and recommendations here are considerations to potentially improve the user experience. Merging with the warden's QA report #209