cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on arbitrarily short TWAP oracle may be inefficient for preventing depeg

[sherlock-admin2]

cergyk

medium

LibUbiquityPool::mintDollar/redeemDollar reliance on arbitrarily short TWAP oracle may be inefficient for preventing depeg

Summary

The ubiquity pool used for minting/burning uAD relies on a twap oracle which can be outdated because the underlying metapool is not updated when calling the ubiquity pool. This would mean that minting/burning will be enabled based on an outdated state when it should have been reverted and inversely

Vulnerability Detail

We can see that LibTWAPOracle::consult computes the average price for uAD on the metapool vs 3CRV. However since it uses the duration since last update as a TWAP duration, it will always get only the value of price at the previous block it was updated at;
https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibTWAPOracle.sol#L80

Let's consider the following example:

metapool initial state at block N:
reserveA: 1000
reserveB: 1000

metapool state at block N+1 (+12 seconds):
reserveA: 1500
reserveB: 500

if we have executed the update at each of these blocks, this means that if we consult the twap at block N+2,
we have:
ts.priceCumulativeLast: [A, B]
priceCumulative: [A+1500*12, B+500*12] (reserve * time_elapsed);
blockTimestamp - ts.pricesBlockTimestampLast = 12;

which means that when we call get_twap_balances the values returned are simply [1500, 500], which are the values of the previous block.

Impact

A malicious user which can control two successive blocks (it is relatively feasible since the merge), can put the twap in any state for the next block:

• Can Dos any minting/burning in the block after the ones he controls
• Can unblock minting/burning for the next block, and depeg uAD further

Code Snippet

Tool used

Manual Review

Recommendation

Use a fixed duration for the TWAP:

uint256[2] memory twapBalances = IMetaPool(ts.pool)
    .get_twap_balances(
        ts.priceCumulativeLast,
        priceCumulative,
        15 MINUTES
    );

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes about TWAP can be manipulated because update function can be called anytime and by anyone, thus TWAP period can be as short as 1 block. It seems like a valid issue but after caeful consideration, it's noticed that the TWAP issue does not come from its period but the logic itself is incorrect, thus marking this as Invalid

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes about TWAP can be manipulated because update function can be called anytime and by anyone, thus TWAP period can be as short as 1 block. It seems like a valid issue but after caeful consideration, it's noticed that the TWAP issue does not come from its period but the logic itself is incorrect, thus marking this as Invalid

[gitcoindev]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

The issue describes about TWAP can be manipulated because update function can be called anytime and by anyone, thus TWAP period can be as short as 1 block. It seems like a valid issue but after caeful consideration, it's noticed that the TWAP issue does not come from its period but the logic itself is incorrect, thus marking this as Invalid

Also invalid then and 'Sponsor Disputed' label should be added? @rndquu @pavlovcik @molecula451

[0x4007]

It probably makes more sense to ask @AuditSea (not sure if this is the corresponding GitHub handle.)

[rndquu]

Yes, it is possible (especially in the early Dollar token stage when market activity is low) to skew the curve's TWAP value since our TWAP (in some cases) may simply take the latest block price thus TWAP can be manipulated with low effort.

This is not critical (i.e. medium severity seems to be valid) since TWAP's price is used only in require() statements on mint and redeem operations but must be fixed.

Not sure what's the best possible solution but the following similar issues are worth to check:

• evmboi32 - TWAP oracle can be easily manipulated #138
• infect3d - TWAP oracle is manipulable as the only time-window requirement is `blockTimestamp #175

[gitcoindev]

@rndquu just to double check, the solution is to set a constant time window for the TWAP price.

A question to Sherlock: should the window be set to 15, 30 min or any other value?

[0x4007]

In my experience with software development, anything time based is better implemented to be event based. Although unfortunately I'm not sure what events would make a great substitute in this case. What if we checked that they aren't consecutive blocks?

[rndquu]

@rndquu just to double check, the solution is to set a constant time window for the TWAP price.

A question to Sherlock: should the window be set to 15, 30 min or any other value?

I think the solution is to use the latest curve's metapool with built-in TWAP oracle and adjustable time window as described here.

the solution is to set a constant time window for the TWAP price

You're right, the solution is to increase it to 15 or 30 minutes.

[rndquu]

Since there is no direct loss of funds the "high" severity doesn't seem to be correct.

[sherlock-admin]

The protocol team fixed this issue in PR/commit ubiquity/ubiquity-dollar#893.

[sherlock-admin]

The Lead Senior Watson signed off on the fix.