Energetic Opaque Elephant - Insufficient Handling of Transfer Fees in DecentralizedIndex::_transferFromAndValidate Causes Legitimate Transactions to Revert

[sherlock-admin2]

Energetic Opaque Elephant

Medium

Insufficient Handling of Transfer Fees in DecentralizedIndex::_transferFromAndValidate Causes Legitimate Transactions to Revert

Summary

The DecentralizedIndex:_transferFromAndValidate function in the contract performs a token transfer and validates the balance update. However, the current implementation uses a strict comparison that does not account for token transfer fees. This can cause legitimate transactions to revert unnecessarily. While the NatSpec mentions fees, the current implementation does not handle them correctly.

Root Cause

The function does not account for tokens that deduct fees during transfers. It validates the balance change using >= _amount, which fails when the received amount is less than _amount due to fees. The NatSpec explicitly states the function should "revert if balances aren’t updated as expected on transfer (i.e., transfer fees, etc.)", but the implementation does not handle this correctly..

Internal Pre-conditions

None

External Pre-conditions

None

Attack Path

Step 1: A token with transfer fees (e.g., 5% fee) is added to the index.

Step 2: A user attempts to bond using this token.

Step 3: _transferFromAndValidate transfers _amount tokens, but the contract receives _amount * 0.95.

Step 4: The check require(balance >= _balanceBefore + _amount) fails, reverting the transaction.

Result: Legitimate users cannot bond with the token.

Impact

Denial-of-Service (DoS): Transactions involving fee-charging tokens will always revert.

Protocol Usability: Limits the protocol’s ability to support popular tokens with transfer fees.

PoC

See PoC below;

The below exposes the _bond function for use in our test

import "../contracts/WeightedIndex.sol";
import "../contracts/interfaces/IDecentralizedIndex.sol";

contract TestWeightedIndex is WeightedIndex {
    /// @notice Public wrapper to call __WeightedIndex_init for testing purposes.
    function publicInit(
        IDecentralizedIndex.Config memory _config,
        address[] memory _tokens,
        uint256[] memory _weights,
        bytes memory _immutables
    ) public {
        __WeightedIndex_init(_config, _tokens, _weights, _immutables);
    }

    /// @notice Returns the number of tokens stored in the indexTokens array.
    function indexTokenCount() public view returns (uint256) {
        return indexTokens.length;
    }

    /// @notice Returns the index stored in the _fundTokenIdx mapping for a given token.
    function getFundTokenIdx(address token) public view returns (uint256) {
        return _fundTokenIdx[token];
    }

     function publicBond(
        address _token,
        uint256 _amount,
        uint256 _amountMintMin,
        address _user
    ) public {
        _bond(_token, _amount, _amountMintMin, _user);
    }
    
}

The below includes the MockFeeToken for testing token with fees

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../contracts/interfaces/IPEAS.sol";


contract MockERC20 {
    string public name;
    string public symbol;
    uint8 public decimals;
    mapping(address => uint256) public balanceOf;
    mapping(address => mapping(address => uint256)) public allowance;

    constructor(string memory _name, string memory _symbol, uint8 _decimals) {
        name = _name;
        symbol = _symbol;
        decimals = _decimals;
    }

    function transfer(address recipient, uint256 amount) public returns (bool) {
        balanceOf[msg.sender] -= amount;
        balanceOf[recipient] += amount;
        return true;
    }

    function approve(address spender, uint256 amount) public returns (bool) {
        allowance[msg.sender][spender] = amount;
        return true;
    }

    // ... Add other mocked functions (like decimals, transferFrom, etc.) as required ...

    function mint(address to, uint256 amount) public {
        balanceOf[to] += amount;
    }

    // function decimals() public view returns (uint8) { // Add decimals function
    //     return decimals;
    // }
}

contract MockFeeToken is MockERC20 {
    uint256 public feePercentage;

    constructor(uint256 _feePercentage) MockERC20("MockFeeToken", "MFT", 18) {
        feePercentage = _feePercentage;
    }

    function transferFrom(address sender, address recipient, uint256 amount) public returns (bool) {
        uint256 fee = amount * feePercentage / 100;
        uint256 amountAfterFee = amount - fee;

        transfer(recipient, amountAfterFee);
        transfer(address(this), fee);

        return true;
    }
}

// interface IERC20Metadata {
//     function decimals() external view returns (uint8);
// }

// Mock contract that DOES NOT implement IERC20Metadata
contract MockNonERC20 {
    // This contract has NO decimals() function
    uint256 public someValue;

    constructor(uint256 _value) {
        someValue = _value;
    }
}


interface IUniswapV2Router02 {
    function WETH() external view returns (address);
    function factory() external view returns (address);
}

contract MockUniswapV2Router is IUniswapV2Router02 {
    address public WETH;
    address public factory;

    constructor(address _weth, address _factory) {
        WETH = _weth;
        factory = _factory;
    }

    // function WETH() external view returns (address) {
    //     return WETH;
    // }

    // function factory() external view returns (address) {
    //     return factory;
    // }

    // ... other functions as needed
}

contract MockPEAS is IPEAS, ERC20 {
    //uint8 public _decimals; // Store decimals as a state variable

    constructor(string memory _name, string memory _symbol, uint8 /* _decimalsValue */) 
        ERC20(_name, _symbol)
    {
        _mint(msg.sender, 10_000_000 * 10 ** 18); // Mint to the deployer for testing
        // Do not store any additional decimals value; rely on ERC20's default.
    }

    function burn(uint256 _amount) external virtual override {
        _burn(msg.sender, _amount); // Burn from the test contract (msg.sender)
        emit Burn(msg.sender, _amount);
    }

    // function decimals() public view virtual override returns (uint8) {
    //     return _decimals; // Return the stored decimals value
    // }

    // Add a mint function for testing purposes:
    function mint(address _to, uint256 _amount) public {
        _mint(_to, _amount);
    }

    // Add a setDecimals function to allow changing the decimals value for testing:
    // function setDecimals(uint8 _newDecimals) public {
    //     _decimals = _newDecimals;
    // }

    // ... other functions as needed for your tests ...
}

contract MockUniswapV2Factory {
    mapping(address => mapping(address => address)) public getPair;

    function setPair(address tokenA, address tokenB, address pairAddress) public {
        getPair[tokenA][tokenB] = pairAddress;
        getPair[tokenB][tokenA] = pairAddress;
    }

    // Simple createPair that deploys a new pair and stores it.
    function createPair(address tokenA, address tokenB) public returns (address pair) {
        MockUniswapV2Pair newPair = new MockUniswapV2Pair(tokenA, tokenB);
        pair = address(newPair);
        setPair(tokenA, tokenB, pair);
    }
}

// Mock Uniswap V2 Pair.
contract MockUniswapV2Pair {
    IERC20 public token0;
    IERC20 public token1;

    constructor(address _tokenA, address _tokenB) {
        token0 = IERC20(_tokenA);
        token1 = IERC20(_tokenB);
    }

    // ... other pair functionality as needed for your tests
}

The test file, include in your test folder

 function test_BondingRevertsWithFeeToken() public {
    // Deploy mock token with configurable transfer fee
         MockFeeToken feeToken = new MockFeeToken(5); // 5% fee     

         // ... (Add feeToken to the index as before)       

         // Mint tokens to user
         uint256 amount = 100e18;
         feeToken.mint(alice, amount);      

         // Approve pod to spend tokens
         vm.startPrank(alice);
         feeToken.approve(address(pod), amount);
         vm.stopPrank();    

         // Get balance of pod before bond
         uint256 balanceBefore = feeToken.balanceOf(address(pod));      

         // Attempt to bond with feeToken
         vm.startPrank(alice);
         vm.expectRevert(); // Expect any revert reason
         podLarge.publicBond(address(feeToken), amount, 0, alice);
         vm.stopPrank();    

         // Get balance of pod after bond
         uint256 balanceAfter = feeToken.balanceOf(address(pod));       

         // Assert that the balance increased by less than the transfer amount
         assertLt(balanceAfter - balanceBefore, amount, "Balance should increase by less than the bond amount due to fees");    

         // Assert that the balance increased (it should increase by the amount after fees)
         assertGt(balanceAfter, balanceBefore, "Balance should increase after bond");       

         // Optionally, get the revert reason and assert it contains "TV" or your custom error message
         // bytes memory revertReason = vm.getRevertReason();
         // assertContains(string(revertReason), "TV"); // Or your custom error message
    }

Mitigation

Modify _transferFromAndValidate to track the actual received amount and use it in downstream logic:

function _transferFromAndValidate(IERC20 _token, address _sender, uint256 _amount) internal returns (uint256 _received) {
    uint256 _balanceBefore = _token.balanceOf(address(this));
    _token.safeTransferFrom(_sender, address(this), _amount);
-      _token.safeTransferFrom(_sender, address(this), _amount);
-       require(_token.balanceOf(address(this)) >= _balanceBefore + _amount, "TV");
+   _received = _token.balanceOf(address(this)) - _balanceBefore;
+   require(_received > 0, "No tokens received"); // Ensure tokens were received
}

Downstream Adjustments:

• Update the _bond function to use _received instead of _amount for calculations (e.g., _totalAssets updates).

• If modifying _bond is not feasible, ensure only tokens without fees are allowed in the index (document this limitation).

Why This Matters
User Trust: Ensures the protocol works as advertised for all supported tokens.

Future-Proofing: Prepares the protocol to handle diverse ERC20 tokens (including those with fees).

This fix aligns the implementation with the NatSpec’s intent and restores functionality for fee-charging tokens.