KingNFT - Execution of orders would revert unexpectedly while ````baseFee```` or ````pairBasedFee```` equals to ````MAX_FEE````

[sherlock-admin]

KingNFT

medium

Execution of orders would revert unexpectedly while baseFee or pairBasedFee equals to MAX_FEE

Summary

The baseFee and pairBasedFee are allowed to set as up to MAX_FEE, but orders would revert unexpectedly due to inconsistent precision processing while calculating fee amount.

Vulnerability Detail

Let's see the case that fee == MAX_FEE (L126) and bFee == MAX_FEE (L133) are allowed.

File: gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol
120:     function setPairBasedFee(
121:         address tokenIn,
122:         address tokenOut,
123:         uint256 fee,
124:         bool applyFee
125:     ) external auth {
126:    if (fee > gladiusReactor.MAX_FEE())
127:        revert InvalidFee();
128:         bytes32 pairHash = getPairHash(tokenIn, tokenOut);
129:         fees[pairHash] = PairBasedFee({applyFee: applyFee, fee: fee});
130:     }
131: 
132:     function setBaseFee(uint256 bFee) external auth {
133:    if (bFee == 0 || bFee > gladiusReactor.MAX_FEE())
134:        revert InvalidFee();
135:    baseFee = bFee;
136:     }
137: 

And we can also find the feeAmount is rounded up while calculating getFeeOutputs()(L082\~L083).

File: gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol
063:     function getFeeOutputs(
064:         ResolvedOrder memory order
065:     ) external view override returns (OutputToken[] memory result) {   

072: 
073:         for (uint256 i = 0; i < order.outputs.length; ++i) {
...
081:             uint256 feeAmount = fee.applyFee
082:                 ? order.outputs[i].amount.mulDivUp(fee.fee, DENOM)
083:                 : order.outputs[i].amount.mulDivUp(baseFee, DENOM);
...
116:     }

But during check stage, it's rounded down (L090), a Off-by-one error occurs here, which would trigger revert on L091.

File: gladius-contracts-internal/src/base/ProtocolFees.sol
039:     function _injectFees(ResolvedOrder memory order) internal view {
...
089:        
090:             if (feeOutput.amount > tokenValue.mulDivDown(MAX_FEE, DENOM)) {
091:                 revert FeeTooLarge(
092:                     feeOutput.token,
093:                     feeOutput.amount,
094:                     feeOutput.recipient
095:                 );
096:             }
...
105:     }

Impact

Execution of orders would revert unexpectedly, break of core functionality.

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L82

Tool used

Manual Review

Recommendation

Keep the consistent round direction while calculating fee amount

Duplicate of #51

[sherlock-admin]

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

0xAadi commented: