Bandit - TWAP Based Off First Transaction In A Block Makes Manipulation Risk Free

[sherlock-admin]

Bandit

high

TWAP Based Off First Transaction In A Block Makes Manipulation Risk Free

Summary

Widely used TWAP Oracles such as Uniswap v3 only take data from the end of the last transaction in a block. This means that an attacker trying to manipulate it has to leave themselves open to large arbitrage losses because they have to leave the pool reserves imbalanced over a block. Dodo's TWAP instead bases the prices after the first transaction, so an attacker can manipulate it and then reset the price back to the orignal price giving them risk free manipulation that cannot be arbitraged.

Vulnerability Detail

In the sellBaseToken and sellQuoteToken functions, _twapUpdate is called after the swap logic. When this is the first swap, liquidity or flash loan action of the block, the TWAP is updateed off the reserves after this swap.

The subsequent actions in a block do not update the TWAP, as the timeElapsed is 0:

        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;

This is actually different from other TWAP designs such as Uniswap v3 TWAP. In those oracles, the price update is based on the price at the end of a block. This makes it resistant manipulation as somebody that wanted to set the TWAP result to an extreme value had to leave it there until the end of the block, in which case it can easily be arbitrage.

In DODO's case, the TWAP can be set to an extreme value simply by bunding these 2 transactions in a flashbots bundle so no arbitrage transactions can be inserted between:

1. making a large swap through the reserves. This changes the recorded TWAP price for the time range since the last update.
2. swap back to exactly the original price. This prevents any losses to arbitrage. Since timeElapsed==0, the TWAP is not changed by this action

Impact

TWAP can be manipulated easily and without risk.

Code Snippet

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

Tool used

Manual Review

Recommendation

Only take prices at the end of a past block. See Uniswap v3's TWAP implementation.

[nevillehuang]

Hi @Skyewwww, any comments on this issue?

[Skyewwww]

We fix this bug in this PR: https://github.com/DODOEX/dodo-gassaving-pool/pull/13

[nevillehuang]

While TWAP is affected, this feature of the protocol is not a core feature of DODO itself. Additionally, TWAP features are deprecated, and will be removed by DODO in future iterations. Any external integrations would be out of scope of this contest.

Sponsor comments:

TWAP was designed in previous versions, but was abandoned before the function was developed. As a result, we don't have any use cases for TWAP, since we only provide view functions for TWAP(can view midPrice and BASE_PRICE_CUMULATIVELAST). Regardless of is_twap=true/false, it will not affect price calculations are used, since we don't have TWAP implementation. To prevent confusion, we will remove the related functions later.

[Banditx0x]

Escalate.

Based on the codebase, it can't be known that the TWAP is out of scope. Additionally, the issue is fixed by the sponsor demonstrating it is completely correct.

[sherlock-admin2]

Escalate.

Based on the codebase, it can't be known that the TWAP is out of scope. Additionally, the issue is fixed by the sponsor demonstrating it is completely correct.

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.

[nevillehuang]

@Banditx0x I understand the concern and was really borderline for this issue, #39 and #96. But after further review, I think it can be deduced from code logic that TWAP is not used at all within DODO GSP and will not affect price calculations in anyway. The sponsor also confirmed with me that they will be removing it completely.

I do agree more information should have been present during the contest phase.

[osmanozdemir1]

I want to point out #96 too, in case of this escalation gets accepted.

Thanks.

[Banditx0x]

@nevillehuang alright thank you

[Czar102]

I agree with @nevillehuang, this would be a future integration issue (as the codebase being audited is not using the TWAP value). Planning to reject the escalation and leave the issue as is.

[Banditx0x]

Yes I see the reasons for the invalidation. This might be a valid medium in other platforms but under Sherlock's rules it may not qualify as medium.

[Czar102]

Result: Low Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• banditx0x: rejected

[CergyK]

We fix this bug in this PR: DODOEX/dodo-gassaving-pool#13

Fix looks good, could additionally mark TWAP related variables as deprecated for clarity (informational)