RebateFi Hook
First Flight #53
Beginner FriendlyDeFi
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

No bounds checking on buyFee / sellFee

s3mvl4d

Description

  • The hook should only accept fee values that are within the protocol’s expected range and representation (dynamic LP fee rates are encoded as parts‑per‑million where 1_000_000 = 100%). Proper validation guards configuration against accidental or malicious values that could break swaps, overcharge users, or render pools unusable.

  • The admin function ChangeFee() allows the owner to set any uint24 value for buyFee and sellFee without checking upper/lower bounds or semantic validity. This enables setting extreme values (e.g., > 1_000_000 or values that protocol does not support) that can cause economic harm or operational disruption.

​
// Root cause in the codebase with @> marks to highlight the relevant section
function ChangeFee(
bool _isBuyFee,
uint24 _buyFee,
bool _isSellFee,
uint24 _sellFee
) external onlyOwner {
if(_isBuyFee) buyFee = _buyFee; // @> no validation
if(_isSellFee) sellFee = _sellFee; // @> no validation
}
​

Risk

Likelihood: Low

  • Occurs during routine governance/operations when an operator sets fees (e.g., testing, hotfixes, or reacting to market conditions).

  • Manifests whenever arbitrary values are used, especially in environments with multiple admins or scripts, since there is no contract‑level safeguard.

Impact: Medium

  • Economic harm: Setting sellFee very high (e.g., >= 1_000_000) can make sells effectively impossible or confiscatory; setting buyFee unintentionally high can overcharge users on buys.

  • Functional breakage / DoS: Unsupported or out‑of‑range values may cause swap reverts, disable liquidity usage, or break assumptions in off‑chain systems that compute fees.

Proof of Concept

  • This test that demonstrates setting an out‑of‑range fee (e.g., 200%):

​
function test_FeesCanBeSetOutOfRange() public {
// Owner sets a clearly invalid fee > 100%
uint24 absurdFee = 2_000_000; // 200%
rebateHook.ChangeFee(false, 0, true, absurdFee);
​
// Contract accepts it; no validation prevents this
(, uint24 sellFee) = rebateHook.getFeeConfig();
assertEq(sellFee, absurdFee, "Sell fee should reflect invalid value due to missing bounds");
}
​

Recommended Mitigation

  • Add explicit bounds checks aligned with parts‑per‑million representation and protocol constraints:

​
function ChangeFee(
bool _isBuyFee,
uint24 _buyFee,
bool _isSellFee,
uint24 _sellFee
) external onlyOwner {
- if(_isBuyFee) buyFee = _buyFee;
- if(_isSellFee) sellFee = _sellFee;
+ // Define a max allowed fee (100% in ppm)
+ uint24 constant MAX_FEE_PPM = 1_000_000;
+ if (_isBuyFee) {
+ require(_buyFee <= MAX_FEE_PPM, "buyFee out of range");
+ buyFee = _buyFee;
+ }
+ if (_isSellFee) {
+ require(_sellFee <= MAX_FEE_PPM, "sellFee out of range");
+ sellFee = _sellFee;
+ }
}
​
Updates

Lead Judging Commences

chaossr Lead Judge 12 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Inverted buy/sell logic when ReFi is currency0, leading to incorrect fee application.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!