sherlock-admin
commented
7 months ago 1 comment(s) were left on this issue during the judging contest.
0xAadi commented:
Closed sherlock-admin closed 7 months ago
sherlock-admin
commented
7 months ago 1 comment(s) were left on this issue during the judging contest.
0xAadi commented:
ArnieSec
commented
7 months ago Escalate
Invalid for a couple reasons
A buyer can end up paying more than his max amount due to execlusivityOverrideBps being applied after validating against the invariant output.amount set by the buyer.
this is wrong the seller is the one who sets output.amount
In handleOverride, the output.amount gets increased with the exclusivityOverrideBps if the filler has no fill right. This would normally be no problem if it got validated, however, this increase of exclusivityOverrideBps happens after the validation of the partition:
this assumption is also wrong, the purpose of the function validatePartition is to ensure that output and input are aligned and quantity does not fall below the sellers fillThreshold. we are validating partition which means we are validating that values are correct after the partition according to the sellers desired outputs/inputs etc... this check was never to protect the buyer, only the seller. since we have shown above these inputs are provided by the seller not buyer...
The buyer would in this case expect his invariant of output.amount to not be exceeded.
once again buyer does not set output.amount this is erroneous his invariants are not broken because he never set them.
All the assumptions in this report are off, invalid
sherlock-admin2
commented
7 months ago Escalate
Invalid for a couple reasons
A buyer can end up paying more than his max amount due to execlusivityOverrideBps being applied after validating against the invariant output.amount set by the buyer.
this is wrong the seller is the one who sets
output.amountIn handleOverride, the output.amount gets increased with the exclusivityOverrideBps if the filler has no fill right. This would normally be no problem if it got validated, however, this increase of exclusivityOverrideBps happens after the validation of the partition:
this assumption is also wrong, the purpose of the function
validatePartitionis to ensure that output and input are aligned and quantity does not fall below the sellers fillThreshold. we are validating partition which means we are validating that values are correct after the partition according to the sellers desired outputs/inputs etc... this check was never to protect the buyer, only the seller. since we have shown above these inputs are provided by the seller not buyer...The buyer would in this case expect his invariant of output.amount to not be exceeded.
once again buyer does not set
output.amountthis is erroneous his invariants are not broken because he never set them.All the assumptions in this report are off, invalid
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.
ArnieSec
commented
7 months ago more evidence that this issue is invalid.
In the readme we get the description of the partition function
partition function is executed during order’s resolution, it mutates input and output amounts by replacing input amount with quantity and calculating output amount, based on the initial exchange rate.
as we can see the main role of partition is to solely mutate inputs and outputs.
from the report it states this
When an order is exactly matched, a buyer can end up paying more than his max amount due to execlusivityOverrideBps
when a partition order is exactly matched it is basically like creating a normal order with no partition, if the filler/buyer where to fill a normal order he would be paying the same amount as a fully matched partition order. So again no impact.
finally let us read the user's recommendation.
Apply the execlusivityOverrideBps before the PartialFillLib.partition function.
if the protocol is to follow this recommendation the seller will be subject to not getting his full override bonus from non exclusive buyers. Imagine this scenario, A seller sets overidebps at 30 and he wishes to receive 200 of token x, he sets fill threshold at 100, meaning his order can be partially filled at half the amount or 100 tokens. If we follow this issues suggestion of adding override before partition, let us say a non exlusive filler wants to fill during exclusivity time, he will pay the 30 bps fee and he only wants to fill 100 tokens instead of the full 200, 200 tokens are now scaled by the fee, then we partition and divide the tokens which now also includes the fee. Essentially we have divided the override fee by half and the seller will lose out on about half the extra tokens he should have received from a non exclusive filler filling during exclusivity time.
bronzepickaxe
commented
7 months ago Hi @ArnieGod, thanks for your input!
We are unsure where you got the idea that the buyer does not specify an output.amount.
Let's look at test/reactors/GladisuReactor.t.sol::test_ExactMatch, which might aid the understanding:
function test_ExactMatch() public {
// (X/Y (sell 100) (buy 200))
GladiusOrder memory ask = defaultAsk();
// (Y/X (sell 200) (buy 100))
GladiusOrder memory bid = defaultBid();
//function defaultAsk() internal view returns (GladiusOrder memory) {
// return customAsk(inputAmount, outputAmount);
//}
//function defaultBid() internal view returns (GladiusOrder memory order) {
// return customBid(outputAmount, inputAmount);
//}
//.. omitted the rest of the test_ExactMatch code
}We added the internal defaultAsk() and defaultBid() functions here. As you can see, the claim you made about the buyer using the same GladiusOrder as the seller does not hold up.
The buyer, naturally, has to specify a GladiusOrder, which includes an output.amount. What you are saying is incorrect - you can not match a GladisuOrder bid by providing the same GladiusOrder bid. The buyer must flip the input and output (read: specified).
/// ----------------------------
/// | X/Y pair |
/// ----------------------------
/// | (X/Y (sell 100) (buy 200)) | <-----
/// | | | match
/// | (Y/X (sell 200) (buy 100)) | <-----
/// ----------------------------The rest of your escalation does not hold up because your escalation builds upon the assumption that the buyer/seller uses the same GladiusOrder, which is not how a buy-and-sell market operates. Asks and bids get matched. You can't match a bid with a bid.
Now, let's look at the code(again).
function partition(
uint256 quantity,
InputToken memory input,
OutputToken[] memory output,
uint256 fillThreshold
) internal pure returns (InputToken memory, OutputToken[] memory) {
_validateThreshold(fillThreshold, input.amount);
uint256 outPart = quantity.mulDivUp(output[0].amount, input.amount);
_validatePartition(
quantity,
outPart,
input.amount,
output[0].amount,
fillThreshold
);
// Mutate amounts in structs.
input.amount = quantity;
output[0].amount = outPart;
return (input, output);
}
function _validatePartition(
uint256 _quantity,
uint256 _outPart,
uint256 _initIn,
uint256 _initOut,
uint256 _fillThreshold
) internal pure {
-> if (_quantity > _initIn || _outPart > _initOut)
revert PartialFillOverflow();
if (_quantity == 0 || _outPart == 0) revert PartialFillUnderflow();
if (_quantity < _fillThreshold) revert QuantityLtThreshold();When the buyer puts in his bid, in partition(), outPart gets validated.
outPart is, in the buyers' case, the amount that the buyer will send. The buyer gets the amount specified in input, and he sends the amount determined in output.
if (_quantity > _initIn || _outPart > _initOut) revert PartialFillOverflow();
Let's look at this line of code again. The transaction will revert if outPart is bigger than output.amount. In other words, if the variable outPart, which gets set to output.amount a few lines after this check, is bigger than the amount specified in output.amount, it will revert with an overflow. This is a good check, you don't want the buyer to pay more than specified in output.amount.
Now, the problem is, as explained in our report, the addition of exactOverrideBps happens after this check:
/// @notice Resolves order into 'GladiusOrder' and applies a decay function.
function resolve(
SignedOrder calldata signedOrder
) internal view override returns (ResolvedOrder memory resolvedOrder) {
GladiusOrder memory order = abi.decode(
signedOrder.order,
(GladiusOrder)
);
_validateOrder(order);
/// @dev Apply decay function.
InputToken memory input = order.input.decay(
order.decayStartTime,
order.decayEndTime
);
OutputToken[] memory outputs = order.outputs.decay(
order.decayStartTime,
order.decayEndTime
);
resolvedOrder = ResolvedOrder({
info: order.info,
input: input,
outputs: outputs,
sig: signedOrder.sig,
hash: order.hash()
});
resolvedOrder.handleOverride(
order.exclusiveFiller,
order.decayStartTime,
order.exclusivityOverrideBps
);
} handleOverride happens at the end of this function. What happens in handleOverride?
function handleOverride(
ResolvedOrder memory order,
address exclusive,
uint256 exclusivityEndTime,
uint256 exclusivityOverrideBps
) internal view {
// if the filler has fill right, we proceed with the order as-is
if (hasFillingRights(exclusive, exclusivityEndTime)) {
return;
}
// if override is 0, then assume strict exclusivity so the order cannot be filled
if (exclusivityOverrideBps == STRICT_EXCLUSIVITY) {
revert NoExclusiveOverride();
}
// scale outputs by override amount
-> OutputToken[] memory outputs = order.outputs;
-> for (uint256 i = 0; i < outputs.length; ) {
-> OutputToken memory output = outputs[i];
-> output.amount = output.amount.mulDivDown(
-> BPS + exclusivityOverrideBps,
-> BPS
-> );
-> unchecked {
-> i++;
}
}
}The output.amount gets increased - which can lead to buyers being charged more than they should.
One of our jobs as SRs is to look at what the code does against what is assumed in the `README.md, so we are not sure about what you are trying to convey in your second comment. If you would like to hold the position that it's normal practice for the buyer to be charged more than he should, that's fine.
To summarize, in no circumstances should a buyer that has specified his output.amount have it exceeded. The input/output invariants are the backbones of projects that use (partial) buy/sell orders. If you want to purchase 1 ETH at an output of 2000 USDC, you should not get charged 2000 USDC + overrideMaxBps.
Thanks!
ArnieSec
commented
7 months ago @bronzepickaxe let us say that the buyer is the exclusive filler. So he will not be charge overrideBps.
Now lets have the exact same scenario but this time the buyer is non exclusive and therefore has to pay overrideBPS
So the seller received the same amount of dai even though the buyer wasnt exclusive. The seller should have received more tokens in this scenario because he set the exclusivityBPS and the filler was non exclusive.
essentially the seller got scammed of his overrideBps extra tokens. Do you see now why your report is erroneous?
ArnieSec
commented
7 months ago if we do not keep override after partition, the protocol would have some serious problems. First of all in the scenario described above where fillthreshold is set to 100 and the seller wants to sell 100 usdc for 200 dai. There is an exact match and a buyer is willing to give up his 200 dai for the 100 usdc. if we do override first, 200 is scaled up by the bps already here the buyers desired output of 200 is exceeded. when we partition since the fillThreshold is set at 100, the output stays above 200 here. The same problem happens.
You are suggesting that the current way is bugged but when we implement your recommendation the same thing happens does it not?
daoio
commented
7 months ago Should be invalid. I've quite misunderstood the report initially, but upon revisting thoroughly I've come to recognize its lack of validity.
While I get the idea of breaking the partition invariant, the thing is that it's a function-level invariant, i.e., validated only in the partition function, what happens with i/o values next doesn't really matter for it.
The next thing is that the exact purpose of exclusivityOverrideBps is to increase output amounts if an order is being executed by a non-exclusive filler. Therefore, regardless of the order of application of these two functions, we're ending up with the same sequence of actions - amounts are mutated by partition and output amount is increased by handleOverride (effectively altering exchange rate, as it's intended).
Also note, that the idea of a buyer and seller is a higher-level concept that doesn't persist on the contract level. While it might be useful to think of them this way for matching purposes, orders aren't interconnected with each other. They're always created by a swapper and executed (or executed in a batch) by a filler.
bronzepickaxe
commented
7 months ago Thank you both for your clarifications.
We used buyer/seller for easy understanding, but we will use taker/asker from now on.
The partition invariant is not broken - the invariant output.amount that is set in the GladiusOrder struct is broken. There can't be efficient trading if the takers have to manually calculate the amount they have to send when exclusivityOverrideBps is applicable, using this amount to approve permit2 and then taking an order.
When a taker takes an ask with a certain input/output amount where exclusivityOverrideBps is applicable, it should not result in the taker sending more tokens than specified in the GladiusOrder struct.
While our recommendation might be invalid, as you both correctly explained since the order of operation does not matter, it does not absolve the report from being valid.
Thanks!
ArnieSec
commented
7 months ago The gladiusOrder struct values are canonical as long as the filler has filling rights. Given that an order can go from being exclusive to not being exlcusive the next block. It would be hard to implement logic to handle these dynamic values in the GladiusOrder struct. Yes you are right there exists friction. But that's how the current codebase just functions. It's the same in uniswapX were override changes the order struct aswell during override. This is not a bug but a design choice. Further iterations/ a v2 of the codebase can improve fluidity of trading further.
But as for the validity of your issue, it is invalid. Just because a market is not optimized doesn't mean that it includes a bug.
The invariant is outputs will remain constant as long as one has filling rights. This invariant holds throughout the codebase.
If you do not have filling rights, you must pay the overrideBps. This is also an invariant.
Fillers are paying the extra fee on top of the agreed upon amount for the chance to fill when they do not have filling rights.
Hope this clears it up
bronzepickaxe
commented
7 months ago Thanks for your response.
Before we submitted the report, we had a look at how UniswapX handled this. However, UniswapX does not have a partition() function. The Rubicon-Finance team added this function to the current codebase.
If you take a look at any of the resolve() functions used by UniswapX, you won't find any mention of a partition function:
https://github.com/Uniswap/UniswapX/blob/main/src/reactors/ExclusiveDutchOrderReactor.sol#L29-L47
function resolve(SignedOrder calldata signedOrder)
internal
view
virtual
override
returns (ResolvedOrder memory resolvedOrder)
{
ExclusiveDutchOrder memory order = abi.decode(signedOrder.order, (ExclusiveDutchOrder));
_validateOrder(order);
resolvedOrder = ResolvedOrder({
info: order.info,
input: order.input.decay(order.decayStartTime, order.decayEndTime),
outputs: order.outputs.decay(order.decayStartTime, order.decayEndTime),
sig: signedOrder.sig,
hash: order.hash()
});
resolvedOrder.handleExclusiveOverride(order.exclusiveFiller, order.decayStartTime, order.exclusivityOverrideBps);
}https://github.com/Uniswap/UniswapX/blob/main/src/reactors/LimitOrderReactor.sol
function resolve(SignedOrder calldata signedOrder)
internal
pure
override
returns (ResolvedOrder memory resolvedOrder)
{
LimitOrder memory limitOrder = abi.decode(signedOrder.order, (LimitOrder));
resolvedOrder = ResolvedOrder({
info: limitOrder.info,
input: limitOrder.input,
outputs: limitOrder.outputs,
sig: signedOrder.sig,
hash: limitOrder.hash()
});
}However, the team added the partition() function and extra checks to the fillThreshold ,quantity input.amount, output.amount:
function partition(
uint256 quantity,
InputToken memory input,
OutputToken[] memory output,
uint256 fillThreshold
) internal pure returns (InputToken memory, OutputToken[] memory) {
-> _validateThreshold(fillThreshold, input.amount);
uint256 outPart = quantity.mulDivUp(output[0].amount, input.amount);
-> _validatePartition(
quantity,
outPart,
input.amount,
output[0].amount,
fillThreshold
);
// Mutate amounts in structs.
input.amount = quantity;
output[0].amount = outPart;
return (input, output);
}
function _validatePartition(
uint256 _quantity,
uint256 _outPart,
uint256 _initIn,
uint256 _initOut,
uint256 _fillThreshold
) internal pure {
-> if (_quantity > _initIn || _outPart > _initOut)
revert PartialFillOverflow();
-> if (_quantity == 0 || _outPart == 0) revert PartialFillUnderflow();
-> if (_quantity < _fillThreshold) revert QuantityLtThreshold();
}The amount.output gets explicitly checked inside this function. This means, given these circumstances, that takers can reasonably expect the amount they will send to never exceed the amount.output inside the GladiusOrder struct. The same thing applies to the asker with regards to the fillThreshold. This is validated inside of _validateThreshold so it is reasonable for the asker to expect his asks to be filled with more than the fillThreshold.
We would agree with your escalations if we were talking about a pure UniswapX fork(read: no added code), because yes, in a pure UniswapX fork, there are no validations done w.r.t. the output.amount and input.amount, so the taker can expect to have some friction and hence it is a design choice made by UniswapX.
However, as you correctly mentioned, in this codebase, friction currently exists in the order amount. And, since the team has chosen to include explicit invariant checks for input.amount and output.amount inside the newly addedpartition() function, the report remains valid.
If this was a bug in code being used by UniswapX, we would've submitted it to them.
Thanks!
ArnieSec
commented
7 months ago You still don't get it I think.
ValidatePartition function just validates that Inputs/Outputs did not over/ under flow during the calculation of outPart. The whole purpose of the function is to serve as a sanity check after we evaluate outPart.
Notice how validatePartition function is placed directly after we calculate the outPart.
This exemplifies that's its sole purpose is to ensure no weird numbers or states come as a consequence of evaluating outPart.
Hey man I'm all for what's fair and just. I have been victim to unjust judgment in contests in the past. But I can assure you this is not the case here.
The issue is invalid.
bronzepickaxe
commented
7 months ago Thanks for your further clarification.
We are going in circles now - it should never be possible for a taker to fill an order where he pays more than output.amount set to 2000 USDC for input.amount 1 ETH due to the validations happening inside of partition(), just like it should never be possible for an asker to have an ask filled with less than the fillThreshold due to the validations happening inside of partition(). This can easily be abused by malicious actors.
We both agreed on the friction currently existing in the execution of orders, which can lead to takers being charged more than the output.amount, but we will have to agree to disagree on the claim that this is a design choice.
As per the judging, we do not feel unfairly treated in this contest. We will let the Lead Judge decide on the outcome.
mstpr
commented
7 months ago This issue also invalid as @ArnieGod states the same "issue" is also exists in UniswapX as you can see here that the exclusivity override happens at the end of the fnction: https://github.com/Uniswap/UniswapX/blob/ebb7d3e46c359339246b14ad110ad42fbf6813d5/src/reactors/ExclusiveDutchOrderReactor.sol#L46
Also, my main take for this contracts are: User should never be underpaid, filler paying more is up to fillers will. If they pay much, then don't fill it. Also, this applies to the filler that is not the exclusive filler, which is even more exotic. The non exclusive filler does not have to fill this trade at all that's why he has the "exclusivityOverrideBps" overpay included. If the filler is ok to take all this, then they can fill no forcing here at all.
Czar102
commented
7 months ago What's the "max amount" mentioned in the title? The amount within the order is the "base amount". If the submitter confused these two values/concepts, I am inclined to accept the escalation and invalidate the issue.
bronzepickaxe
commented
7 months ago @Czar102 Thank you for your question.
What we meant by maxAmount is a taker filling an order maximally.
If a taker specifies the quantity and GladiusOrder.output.amount when taking an order, he should not expect these values to overflow due to the checks added by this protocol inside partition() (which is not comparable to UniswapX since they don't have a function that checks for underflows or overflows of the GladiusOrder.output.amount/quantity). The same way the asker should not expect people to fill his ask under the threshold. Both these checks happen in partition().
The comparison with UniswapX mentioned a few comments ago would be correct if the team did not add additional validation before applying the exclusivityOvverideBps, but they did in partition() alongside validations pertaining the threshold set by the asker.
Thanks!
Czar102
commented
7 months ago Hey @bronzepickaxe, I'm not sure what overflows you are talking about refer to – is that filling an order above its output.amount?
@bronzepickaxe Could you expand on the interface of the filler? I would like to explore in more depth why is there, in your opinion, an expectation for an argument to be considered a "max amount" (like slippage protection).
bronzepickaxe
commented
7 months ago Hi @Czar102, thanks for your reply!
We want to preface this by saying that we will summarise our stance on why we think our issue is valid. We hope that this will answer your questions. We apologize for the lengthy answer in advance and hope our answer aids in completing the judging process for this contest.
Firstly, we would like to acknowledge that the recommendation in our report is insufficient. Other Watsons have correctly addressed this - however, an insufficient recommendation is not a reason to invalidate an issue, which is one of the arguments made during the escalations.
Secondly, to answer your first question, when we mention an overflow, we are talking about the checks happening in the newly added partition() function:
function partition(
uint256 quantity,
InputToken memory input,
OutputToken[] memory output,
uint256 fillThreshold
) internal pure returns (InputToken memory, OutputToken[] memory) {
_validateThreshold(fillThreshold, input.amount);
uint256 outPart = quantity.mulDivUp(output[0].amount, input.amount);
_validatePartition(
quantity,
outPart,
input.amount,
output[0].amount,
fillThreshold
);
// Mutate amounts in structs.
input.amount = quantity;
output[0].amount = outPart;
return (input, output);
}
This functionality does not exist in UniswapX, nor do they have the validation checks happening inside the partition() function. This is important to note because most arguments made during the escalations used UniswapX as a basis for their argument.
If we take a closer look at the partition() function, we see two functions that do validation:
_validateThreshold_validatePartitionThe first check, _validateThreshold:
function _validateThreshold(
uint256 _fillThreshold,
uint256 _inAmt
) internal pure {
if (_fillThreshold > _inAmt) revert InvalidThreshold();
}The second check, _validatePartition:
function _validatePartition(
uint256 _quantity,
uint256 _outPart,
uint256 _initIn,
uint256 _initOut,
uint256 _fillThreshold
) internal pure {
if (_quantity > _initIn || _outPart > _initOut)
revert PartialFillOverflow();
if (_quantity == 0 || _outPart == 0) revert PartialFillUnderflow();
if (_quantity < _fillThreshold) revert QuantityLtThreshold();
}It is important to mention these two in unison because one comment mentioned the following:
While I get the idea of _breaking the partition invariant_, the thing is that it's a function-level invariant, i.e., validated only in the `partition` function, what happens with i/o values next doesn't really matter for it.If this was true, does this mean that the _validateThreshold check, which also resides in the partition(), is just an arbitrary invariant check? This nullifies the sole purpose of using a `fillThreshold:
struct GladiusOrder {
//.. omitted code
-> // Minimum amount of input token, that can be partially filled by taker.
-> uint256 fillThreshold;
}Furthermore, additional checks happen inside _validatePartition:
if (_quantity == 0 || _outPart == 0) revert PartialFillUnderflow();
if (_quantity < _fillThreshold) revert QuantityLtThreshold();fillThreshold. _outPart is checked to not be 0.We do not agree that these are merely invariant checks on a function-level. There are no other checks happening w.r.t the quantity, fillThreshold, and input.amount/output.amount in the whole execute() flow. If they truly do not matter as per the Teams' comment, then our recommendation would be to remove these checks.
Thirdly, to answer your second question, let's take a look at _validatePartition, more specifically, this line of code:
if (_quantity > _initIn || _outPart > _initOut)
revert PartialFillOverflow();The _outPart variable here is:
uint256 outPart = quantity.mulDivUp(output[0].amount, input.amount);The taker provides the quantity variable, which gets multiplied and divided by the output.amount and input.amount, both provided by the taker. This _outPart is then checked against output.amount for a potential overflow and revert if there is an overflow.
Taking all the aforementioned into account, we argue that a taker can reasonably expect to not have more funds taken out of his wallet other than what has been specified during his initial execute() call, since partition() has invariant checks that should prevent this from happening. The same applies to the asker, who sets a fillThreshold, should never have his asks filled below this fillThreshold, since this invariant check happens inside partition().
We hope that this answers your questions and we thank everyone for their input regarding this issue.
Thanks!
Czar102
commented
7 months ago I really appreciate the thorough explanation. I see, underflow/overflow were referring to the revert selectors used.
I would look at the quantity argument as a measure of the extent to which the order is to be filled (quantity / input.amount), and this is why there are checks in place to validate it. This is further confirmed by the NatSpec comment describing the quantity argument:
quantity- amount in the form ofinput.tokento buy from an order.
The definition revolves around the extent of filling of an order. Hence, if the filler is non-exclusive, the fact that this partition is filled at a premium should not be influencing this parameter.
I understand your position that the "internal function invariant" is broken. I hope you also see mine and sponsor's point of view. In the end, it's a question of whether there is a reasonable expectation of not paying more for a partition fill than quantity, and I don't think so.
Hence, I'm planning to accept the escalation and invalidate the issue.
bronzepickaxe
commented
7 months ago Hi @Czar102, we can see the point of view of you and the sponsor. There are indeed good arguments to be made for both sides - we appreciate your time and thank you for going through our lengthy explanation.
Czar102
commented
7 months ago Result: Invalid Unique
sherlock-admin2
commented
7 months ago
Fassi_Security
high
When an order is exactly matched, a buyer can end up paying more than his max amount due to execlusivityOverrideBps
Summary
A buyer can end up paying more than his max amount due to
execlusivityOverrideBpsbeing applied after validating against the invariantoutput.amountset by the buyer.Vulnerability Detail
Let's consider the following scenario(graph taken from
test/reactors/GladiusReactor.t.sol):The problem here is, which is unaccounted for in the test suite due to modified mocks being used instead the real contract, is that the buyer will end up spending more than
200 DAIdue tohandleOverridebeing applied after the validation that happens inPartialFillLib.partition:In
handleOverride, theoutput.amountgets increased with theexclusivityOverrideBpsif the filler has no fill right. This would normally be no problem if it got validated, however, this increase ofexclusivityOverrideBpshappens after the validation of the partition:This means that the
output.amount + overrideBps applied, nullifies this invariant check that happens above:if (_quantity > _initIn || _outPart > _initOut) revert PartialFillOverflow();outPart, which gets checked against_initOut(output.amount), is being assured that its not more than the output.amount. After this check,output.amountgets set tooutPart.Unfortunately this validation happens before applying the
excessOverrideBps, which means that this check gets broken and the user ends up paying more thanoutput.amount.Impact
To continue with our example above, lets say our seller is a malicious user who has put the
execlusivityOverrideBpsat high amount. The buyer would in this case expect his invariant ofoutput.amountto not be exceeded. However, if there isexclusivityOverrideBpsset, the buyer will pay more thanoutput.amount, breaking this critical invariant that should never be broken. If a user puts a limit order to buy 100 USDC using 200 DAI, it should at max take 200 DAI, not 200 DAI + overrideBps.One might say a person will need to approve
permit2the exact amount before executing each trade, however, in reality, people just approve anx amountof tokens topermit2so they can efficiently trade.Furthermore, one might say, the buyer can see the
exclusivityOverrideBps, which is true, however, theexclusivityOverrideBpsadded should never exceedamount.output, which is the case since the project checks for this invariant to hold inside_validatePartition.Code Snippet
https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/main/gladius-contracts-internal/src/lib/PartialFillLib.sol#L119-L130
Tool used
Manual Review
Recommendation
Apply the
execlusivityOverrideBpsbefore thePartialFillLib.partitionfunction.