xiaoming90 - Lack of slippage control in buy order

[sherlock-admin4]

xiaoming90

high

Lack of slippage control in buy order

Summary

The lack of slippage control in the buy order could result in users paying a higher price than they are willing to.

Vulnerability Detail

When users submit the PowerToken.buy transaction to purchase Power Token from the auction, they can define the expiryEpoch_. The expiryEpoch_ ensures that the buy transaction will only be executable till expiryEpoch_ epoch. Beyond expiryEpoch_ epoch, the transaction will revert as per Line 132 below.

Typically, users will define the buy transaction as valid until the end of the current transfer epoch. However, in some cases, some users would define a expiryEpoch_ that allows their buy order to span across multiple transfer epochs.

Assume that Bob defines a buy order with an expiryEpoch_ that allows his order to span across three (3) transfer epochs.

Assume that the price of the auction starts at 100 for all three transfer epochs for simplicity's sake.

Around halfway through the auction of the first transfer epoch, the price reached 50, and Bob decided to submit a buy order. Bob is only willing to pay a price of up to 50 (Maximum price of 50) for the power tokens in the auction. Suppose a buy order only spans a single transfer epoch. In that case, there is no need for any slippage control such as maxPrice because the later the TX is executed, the cheaper the power tokens will be, which actually benefits Bob.

However, it is different if a buy order spans across multiple transfer epochs. Assume Bob's buy order is not executed in the first transfer epoch due to many users attempting to purchase during the first auction. At the start of the second transfer epoch, when the price is 90, Bob's buy order might be executed, and he has to pay a price of 90 for the power tokens. This price exceeds the maximum price he is willing to pay (50).

https://github.com/sherlock-audit/2023-10-mzero/blob/main/ttg/src/PowerToken.sol#L124

File: PowerToken.sol
124:     function buy(
125:         uint256 minAmount_,
126:         uint256 maxAmount_,
127:         address destination_,
128:         uint16 expiryEpoch_
129:     ) external returns (uint240 amount_, uint256 cost_) {
130:         // NOTE: Buy order has an epoch-based expiration logic to avoid user's unfair purchases in subsequent auctions.
131:         //       Order should be typically valid till the end of current transfer epoch.
132:         if (_clock() > expiryEpoch_) revert ExpiredBuyOrder();
133:         if (minAmount_ == 0 || maxAmount_ == 0) revert ZeroPurchaseAmount();
134: 
135:         uint240 amountToAuction_ = amountToAuction();
136:         uint240 safeMinAmount_ = UIntMath.safe240(minAmount_);
137:         uint240 safeMaxAmount_ = UIntMath.safe240(maxAmount_);
138: 
139:         amount_ = amountToAuction_ > safeMaxAmount_ ? safeMaxAmount_ : amountToAuction_;
140: 
141:         if (amount_ < safeMinAmount_) revert InsufficientAuctionSupply(amountToAuction_, safeMinAmount_);
142: 
143:         emit Buy(msg.sender, amount_, cost_ = getCost(amount_));
144: 
145:         _mint(destination_, amount_);
146: 
147:         // NOTE: Not calling `distribute` on vault since anyone can do it, anytime, and this contract should not need to
148:         //       know how the vault works
149:         if (!ERC20Helper.transferFrom(cashToken(), msg.sender, vault, cost_)) revert TransferFromFailed();
150:     }

Impact

Lack of slippage control, resulting in users having to pay a higher price than they are willing to.

Code Snippet

https://github.com/sherlock-audit/2023-10-mzero/blob/main/ttg/src/PowerToken.sol#L124

Tool used

Manual Review

Recommendation

Consider implementing the following slippage control to give users the option to halt the transaction if it exceeds the price they are willing to pay.

function buy(
    uint256 minAmount_,
    uint256 maxAmount_,
    address destination_,
+   uint256 maxPrice_,    
    uint16 expiryEpoch_
) external returns (uint240 amount_, uint256 cost_) {
    emit Buy(msg.sender, amount_, cost_ = getCost(amount_));
+   if (cost_ > maxPrice_) revert CostExceeded();

[nevillehuang]

I believe issue 7.7 description in chainsecurity is sufficient to invalidate issue #58

The function PowerToken.buy() does not allow users to specify an expiry timestamp, which would prevent a transaction to execute at a later time. Currently, it is possible that user's transaction gets executed at a future transfer epoch and potentially buys tokens with a price higher than originally intended.

[xiaoming9090]

Escalate.

The "ChainSecurity Issue 7.7” should not be used to invalidate Issue 58 as a known issue because their impact is the same. If there are two issues or root causes that coincidentally have the same impact (resulting in buying tokens with a price higher than the original intent), they are still considered two unique issues. Only issues that have the exact root causes can be grouped together. However, in this case, the root cause is different.

In "ChainSecurity Issue 7.7”, the root cause is due to the lack of expiry, which could potentially lead to users buying tokens with a price higher than originally intended. The MZERO team has attempted to remediate the ChainSecurity issue by implementing the expiry parameter within the buy function.

However, this report highlights that having the expiry epoch alone is insufficient to remediate the issue, as users are allowed to define the expiry epoch across multiple auctions, which leads to this issue. The root cause of this issue is the lack of a maxPrice parameter that allows users to restrict the amount they need to pay during an auction.

The protocol team might expect the users or the front end (UI) to always set the expiry epoch to the current epoch. However, in the context of this audit contest, the risks that users could potentially encounter if they do not set the expiry epoch to the current epoch were not explicitly highlighted in the contest's known issues, source code's comments, or documentation. It is unrealistic for the users to be aware of all the potential risks of setting the expiry epoch to a value longer than the current epoch without explicitly documenting them. Regular users who have not inspected the contract's source code in detail would not be aware that setting the expiry epoch to a value longer than the current epoch would lead to a loss of assets as the expiry parameter by the name itself only indicate how long the transaction remain valid throughout a period.

Thus, this issue should remain valid in the context of this audit contest.

[sherlock-admin2]

Escalate.

The "ChainSecurity Issue 7.7” should not be used to invalidate Issue 58 as a known issue because their impact is the same. If there are two issues or root causes that coincidentally have the same impact (resulting in buying tokens with a price higher than the original intent), they are still considered two unique issues. Only issues that have the exact root causes can be grouped together. However, in this case, the root cause is different.

In "ChainSecurity Issue 7.7”, the root cause is due to the lack of expiry, which could potentially lead to users buying tokens with a price higher than originally intended. The MZERO team has attempted to remediate the ChainSecurity issue by implementing the expiry parameter within the buy function.

However, this report highlights that having the expiry epoch alone is insufficient to remediate the issue, as users are allowed to define the expiry epoch across multiple auctions, which leads to this issue. The root cause of this issue is the lack of a maxPrice parameter that allows users to restrict the amount they need to pay during an auction.

The protocol team might expect the users or the front end (UI) to always set the expiry epoch to the current epoch. However, in the context of this audit contest, the risks that users could potentially encounter if they do not set the expiry epoch to the current epoch were not explicitly highlighted in the contest's known issues, source code's comments, or documentation. It is unrealistic for the users to be aware of all the potential risks of setting the expiry epoch to a value longer than the current epoch without explicitly documenting them. Regular users who have not inspected the contract's source code in detail would not be aware that setting the expiry epoch to a value longer than the current epoch would lead to a loss of assets as the expiry parameter by the name itself only indicate how long the transaction remain valid throughout a period.

Thus, this issue should remain valid in the context of this audit contest.

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]

The whole purpose of including expiryEpoch is to act as a form of deadline/slippage control for any potential pending governance actions/price actions. This can be seen by sponsors comment here and here

I believe users not understanding the risks involved with this parameter is not adequate to validate the issue as a security risk.

[xiaoming9090]

The whole purpose of including expiryEpoch is to act as a form of deadline/slippage control for any potential pending governance actions/price actions. This can be seen by sponsors comment here and here

I would like to reiterate the following point made in my earlier comment (https://github.com/sherlock-audit/2023-10-mzero-judging/issues/58#issuecomment-2041019348) to address the above response:

The protocol team might expect the users or the front end (UI) to always set the expiry epoch to the current epoch. However, in the context of this audit contest, the risks that users could potentially encounter if they do not set the expiry epoch to the current epoch were not explicitly highlighted in the contest's known issues, source code's comments, or documentation. It is unrealistic for the users to be aware of all the potential risks of setting the expiry epoch to a value longer than the current epoch without explicitly documenting them. Regular users who have not inspected the contract's source code in detail would not be aware that setting the expiry epoch to a value longer than the current epoch would lead to a loss of assets as the expiry parameter by the name itself only indicate how long the transaction remain valid throughout a period.

[deluca-mike]

I don't think it is fair to say that only risks that are well documented are acceptable, especially when if you choose to not read the contract's source code, then you better use the first party app, which sets correct values (and that's still not ultimately prudent, but usually socially acceptable). If you don't read swap contacts and decide to manually call the contract with an allowed slippage of 100%, then that's equally the fault of the user.

[Evert0x]

Planning to reject escalation and keep issue invalid.

I believe the expiryEpoch_ function parameter is sufficient to mitigate the slippage attack vector.

[Evert0x]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: rejected