pontifex - DoS due to unexpected revert in twap update

[sherlock-admin2]

pontifex

high

DoS due to unexpected revert in twap update

Summary

Due to an underflow error at the GSPVault._twapUpdate function the protocol functionality will be blocked. So users will not be able to redeem shares.

Vulnerability Detail

There is a normalization of the block.timestamp value to uint32 at the GSPVault._twapUpdate function which works well in solidity 0.6.9 but in recent versions will throw an underflow error.
https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPVault.sol#L89-L90
In case the _IS_OPEN_TWAP_ parameter will be initialized as true this issue can cause a permanent asset blocking at the pool when block.timestamp exceeds type(uint32).max.

Impact

Permanent asset blocking at the pool.

Code Snippet

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPVault.sol#L89-L90

Tool used

Manual Review

Recommendation

Consider using unchecked braces for this calculation:

    function _twapUpdate() internal {
        // blockTimestamp is the timestamp of the current block
        uint32 blockTimestamp = uint32(block.timestamp % 2**32);
        // timeElapsed is the time elapsed since the last update
+       unchecked {
            uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMP_LAST_;
+       }
        // if timeElapsed is greater than 0 and the reserves are not 0, update the twap price
        if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {
            _BASE_PRICE_CUMULATIVE_LAST_ += getMidPrice() * timeElapsed;
        }
        // update the last block timestamp
        _BLOCK_TIMESTAMP_LAST_ = blockTimestamp;
    }

[nevillehuang]

Low severity, this would take 80+ years for this underflow to occur

[ChechetkinVV]

Escalate

It appears from the implementation of the GSPVault._twapUpdate function that the protocol team paid due attention to handling such an event, even if it does not occur until 80+ years later. Thus, the expected behavior of the function is violated, which will lead to DoS of the protocol.

Obviously, if the issue is not fixed, this event will occur with 100% probability. The impression of a low probability of an event due to delay probably arises from assumptions that the bug will be found and fixed or the protocol/network will not last that long. In my opinion, it is incorrect to rely on this.

Based on the above arguments, I disagree with Low. The severity should remain High or at least Medium.

[nevillehuang]

@ChechetkinVV I have no further input for this argument 😄, thanks for making my day. This should remain low severity.

[sherlock-admin2]

Escalate

It appears from the implementation of the GSPVault._twapUpdate function that the protocol team paid due attention to handling such an event, even if it does not occur until 80+ years later. Thus, the expected behavior of the function is violated, which will lead to DoS of the protocol.

Obviously, if the issue is not fixed, this event will occur with 100% probability. The impression of a low probability of an event due to delay probably arises from assumptions that the bug will be found and fixed or the protocol/network will not last that long. In my opinion, it is incorrect to rely on this.

Based on the above arguments, I disagree with Low. The severity should remain High or at least Medium.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Czar102]

the protocol team paid due attention to handling such an event

Could you elaborate?
I can't see where are they paying attention to this or where is it shown that they want the smart contracts to work after 80+ years.

[ChechetkinVV]

the protocol team paid due attention to handling such an event

Could you elaborate? I can't see where are they paying attention to this or where is it shown that they want the smart contracts to work after 80+ years.

Yes, sure. This is a contract update. It was copied from the old version of Solidity, where the underflow was left. But here they clearly forgot to add unchecked parentheses. In fact, no changes were made to this function and in Solidity 0.8.16 it works differently from 0.6.9.

[nevillehuang]

@Czar102 @ChechetkinVV Lets put all the timing stuff aside which I believe is already sufficient to keep low/invalid. This issue impacts twap values which is not used for price calculations, so doesn’t matter, should be invalid in consistent with issues like #80

[Czar102]

It was copied from the old version of Solidity, where the underflow was left. But here they clearly forgot to add unchecked parentheses. In fact, no changes were made to this function and in Solidity 0.8.16 it works differently from 0.6.9.

It seems like the team doesn't care about it.

Lets put all the timing stuff aside which I believe is already sufficient to keep low/invalid. This issue impacts twap values which is not used for price calculations, so doesn’t matter, should be invalid in consistent with issues like #80

I think this function may be invoked in a swap, so this will DoS the protocol.

[ChechetkinVV]

It seems like the team doesn't care about it.

I remembered why I thought that they wanted to process this case. The team chose to cast block.timestamp to uint32 in a way that explicitly indicates that block.timestamp may be greater than 2**32.

        uint32 blockTimestamp = uint32(block.timestamp % 2**32);

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/af43d39f6a89e5084843e196fc0185abffe6304d/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPVault.sol#L88

[Czar102]

Taking the value modulo 2 ** 32 just trims the value to the uint32 size. It has no effect on the result of the casting. I don't think it's sufficient to consider sponsors intending the contracts to work for overflowing timestamps, though it's quite close. This could be a result of them making mathematically sure that the value being casted won't overflow (not that it matters).

I know this is to an extent subjective and I guess some judgments need to be. What further makes me feel safe considering it a low is a long time we'd need to wait for the bug to uncover.

Hence, I am planning to reject the escalation and leave the issue as is.

[ChechetkinVV]

@Czar102, Thank you for your time!

[Czar102]

Result:
Low
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ChechetkinVV: rejected