BugPull - Broken core contract functionality `UniswapImplementation::setFeeExemption` making `exemptionFee` is never useable

[sherlock-admin3]

BugPull

Medium

Broken core contract functionality UniswapImplementation::setFeeExemption making exemptionFee is never useable

Summary

In oreder to set a FeeExemption we assign a boolean flag to know if there is a exemptionfee or not but doing so with 0xFFFFFF making misinterpretation and could override storage and fail to assign the fee which appears in

• UniswapImplementation::setFeeExemption
• UniswapImplementation::removeFeeExemption
• UniswapImplementation::getFee making feeExemption never usable

Root Cause

Using this check UniswapImplementation.sol#L738-L739 feeOverrides[_beneficiary] = uint48(_flatFee) << 24 | 0xFFFFFF; in UniswapImplementation::setFeeExemption

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Knowing how feeOverrides[_beneficiary] is setted in UniswapImplementation::setFeeExemption

feeOverrides[_beneficiary] = uint48(_flatFee) << 24 | 0xFFFFFF; //@audit those bitwise manipulation seems wrong with example _flatfee of 0

• uint48(_flatFee):
  • Binary: 000000000000000000000000000000000000000000000000 (48 bits of 0)
• uint48(_flatFee) << 24:
  • Binary: 000000000000000000000000000000000000000000000000 (unchanged, as we're shifting 0s)
• 0xFFFFFF:
  • Binary: 000000000000000000000000111111111111111111111111 (24 1's in the lower bits)
• Result of | operation:

   000000000000000000000000000000000000000000000000
   |
   000000000000000000000000111111111111111111111111
   -----------------------------------------------
   000000000000000000000000111111111111111111111111

• Final result:
  • Binary: 000000000000000000000000111111111111111111111111
  • Hexadecimal: 0x00FFFFFF
  • Decimal: 16777215

in getFee the check will always be false

File: UniswapImplementation.sol
698:     function getFee(PoolId _poolId, address _sender) public view returns (uint24 fee_) { 
////code
711:@>       if (uint24(swapFeeOverride & 0xFFFFFF) == 1) {
712:             // We can then extract the original uint24 fee override and apply this, only if

calculations

   000000000000000000000000111111111111111111111111 (swapFeeOverride)
   &
   000000000000000000000000111111111111111111111111 (0xFFFFFF)
   -----------------------------------------------
   000000000000000000000000111111111111111111111111

• Result: 0xFFFFFF (16777215 in decimal) which doesn't equal to 1.

• This is also applied to UniswapImplementatoin::removeFeeExemption

  File: UniswapImplementation.sol
  749:     function removeFeeExemption(address _beneficiary) public onlyOwner {
  750:         // Check that a beneficiary is currently enabled
  751:@>       uint24 hasExemption = uint24(feeOverrides[_beneficiary] & 0xFFFFFF);
  752:         if (hasExemption != 1) {

exemptionFee can't be used due to wrong bitwise operation.

Impact

We can't set exemptionFee

PoC

run these tests before and after the modificaion

• UniswapImplementationTest::test_CanSetFeeExemption
• UniswapImplementationTest::test_CanGetFeeWithExemptFees
• UniswapImplementationTest::test_CanRemoveFeeExemption

    function test_CanSetFeeExemption(address _beneficiary, uint24 _validFee) public {
        // Ensure that the valid fee is.. well.. valid
        vm.assume(_validFee.isValid());

        // Confirm that the position does not yet exist
        (uint24 storedFee, bool enabled) = _decodeEnabledFee(uniswapImplementation.feeOverrides(_beneficiary));
        assertEq(storedFee, 0);
        assertEq(enabled, false);

        vm.expectEmit();
        emit UniswapImplementation.BeneficiaryFeeSet(_beneficiary, _validFee);
        uniswapImplementation.setFeeExemption(_beneficiary, _validFee);

        // Get our stored fee override
        uint48 feeOverride = uniswapImplementation.feeOverrides(_beneficiary);

        // Confirm the fee override is what we expect it to be
        assertEq(feeOverride, _encodeEnabledFee(_validFee, true));

        // Confirm that the decoded elements are correct
        (storedFee, enabled) = _decodeEnabledFee(feeOverride);
        assertEq(storedFee, _validFee);
        assertEq(enabled, true);
    }

Mitigation

File: UniswapImplementation.sol
-738:         feeOverrides[_beneficiary] = uint48(_flatFee) << 24 | 0xFFFFFF;
+738:         feeOverrides[_beneficiary] = uint48(_flatFee) << 24 | 1; 

using 1 will not cuse problem and feeExemption will be used normally

If we used 1 instead of 0xFFFFFF in setExemptionFee

   000000000000000000000000000000000000000000000000 (swapFeeOverride)
   |
   000000000000000000000000000000000000000000000001 (0xFFFFFF)
   -----------------------------------------------
   000000000000000000000000000000000000000000000001

This is the calculation of and operation in getFee and removeFeeExemption

   000000000000000000000000000000000000000000000001 (swapFeeOverride)
   &
   000000000000000000000000111111111111111111111111 (0xFFFFFF)
   -----------------------------------------------
   000000000000000000000000000000000000000000000001

• Result: 0xFFFFFF (1 in decimal) which equal to 1.