[Out of Scope] ReferralFeeReceiver - anyone can steal all the funds that belong to ReferralFeeReceiver Unverified
Note: This issue was raised in components that were being affected by the scope reduction as outlined in the section “Scope” and are, therefore, only shallowly validated. Nevertheless, we find it important to communicate such potential findings and ask the client to further investigate.\\nThe `ReferralFeeReceiver` receives pool shares when users `swap()` tokens in the pool. A `ReferralFeeReceiver` may be used with multiple pools and, therefore, be a lucrative target as it is holding pool shares.\\nAny token or `ETH` that belongs to the `ReferralFeeReceiver` is at risk and can be drained by any user by providing a custom `mooniswap` pool contract that references existing token holdings.\\nIt should be noted that none of the functions in `ReferralFeeReceiver` verify that the user-provided `mooniswap` pool address was actually deployed by the linked `MooniswapFactory`. The factory provides certain security guarantees about `mooniswap` pool contracts (e.g. valid `mooniswap` contract, token deduplication, tokenA!=tokenB, enforced token sorting, …), however, since the `ReferralFeeReceiver` does not verify the user-provided `mooniswap` address they are left unchecked.\\nAdditional Notes\\n`freezeEpoch` - (callable by anyone) performs a `pool.withdraw()` with the `minAmounts` check being disabled. This may allow someone to call this function at a time where the contract actually gets a bad deal.\\n`trade` - (callable by anyone) can intentionally be used to perform bad trades (front-runnable)\\n`trade` - (callable by anyone) appears to implement inconsistent behavior when sending out `availableBalance`. `ETH` is sent to `tx.origin` (the caller) while tokens are sent to the user-provided `mooniswap` address.\\n```\\nif (path[0].isETH()) {\\n    tx.origin.transfer(availableBalance);  // solhint-disable-line avoid-tx-origin\\n} else {\\n    path[0].safeTransfer(address(mooniswap), availableBalance);\\n}\\n```\\n\\nmultiple methods - since `mooniswap` is a user-provided address there are a lot of opportunities to reenter the contract. Consider adding reentrancy guards as another security layer (e.g. `claimCurrentEpoch` and others).\\nmultiple methods - do not validate the amount of tokens that are returned, causing an evm assertion due to out of bounds index access.\\n```\\nIERC20[] memory tokens = mooniswap.getTokens();\\nuint256 token0Balance = tokens[0].uniBalanceOf(address(this));\\nuint256 token1Balance = tokens[1].uniBalanceOf(address(this));\\n```\\n\\nin `GovernanceFeeReceiver` anyone can intentionally force unwrapping of pool tokens or perform swaps in the worst time possible. e.g. The checks for `withdraw(..., minAmounts)` is disabled.\\n```\\nfunction unwrapLPTokens(Mooniswap mooniswap) external validSpread(mooniswap) {\\n    mooniswap.withdraw(mooniswap.balanceOf(address(this)), new uint256[](0));\\n}\\n\\nfunction swap(IERC20[] memory path) external validPath(path) {\\n    (uint256 amount,) = \\_maxAmountForSwap(path, path[0].uniBalanceOf(address(this)));\\n    uint256 result = \\_swap(path, amount, payable(address(rewards)));\\n    rewards.notifyRewardAmount(result);\\n}\\n```\\n\\nA malicious user can drain all token by calling `claimFrozenEpoch` with a custom contract as `mooniswap` that returns a token address the `ReferralFeeReceiver` contracts holds token from in `IERC20[] memory tokens = mooniswap.getTokens();`. A subsequent call to `_transferTokenShare()` will then send out any amount of token requested by the attacker to the attacker-controlled address (msg.sender).\\nLet's assume the following scenario:\\n`ReferralFeeReceiver` holds `DAI` token and we want to steal them.\\nAn attacker may be able to drain the contract from `DAI` token via `claimFrozenToken` if\\nthey control the `mooniswap` address argument and provide a malicious contract\\n`user.share[mooniswap][firstUnprocessedEpoch] > 0` - this can be arbitrarily set in `updateReward`\\n`token.epochBalance[currentEpoch].token0Balance > 0` - this can be manipulated in `freezeEpoch` by providing a malicious `mooniswap` contract\\nthey own a worthless `ERC20` token e.g. named `ATTK`\\nThe following steps outline the attack:\\nThe attacker calls into `updateReward` to set `user.share[mooniswap][currentEpoch]` to a value that is greater than zero to make sure that `share` in `claimFrozenEpoch` takes the `_transferTokenShare` path.\\n```\\nfunction updateReward(address referral, uint256 amount) external override {\\n    Mooniswap mooniswap = Mooniswap(msg.sender);\\n    TokenInfo storage token = tokenInfo[mooniswap];\\n    UserInfo storage user = userInfo[referral];\\n    uint256 currentEpoch = token.currentEpoch;\\n\\n    // Add new reward to current epoch\\n    user.share[mooniswap][currentEpoch] = user.share[mooniswap][currentEpoch].add(amount);\\n    token.epochBalance[currentEpoch].totalSupply = token.epochBalance[currentEpoch].totalSupply.add(amount);\\n\\n    // Collect all processed epochs and advance user token epoch\\n    \\_collectProcessedEpochs(user, token, mooniswap, currentEpoch);\\n}\\n```\\n\\nThe attacker then calls `freezeEpoch()` providing the malicious `mooniswap` contract address controlled by the attacker.\\nThe malicious contract returns token that is controlled by the attacker (e.g. ATTK) in a call to `mooniswap.getTokens();`\\nThe contract then stores the current balance of the attacker-controlled token in `token0Balance/token1Balance`. Note that the token being returned here by the malicious contract can be different from the one we're checking out in the last step (balance manipulation via `ATTK`, checkout of `DAI` in the last step).\\nThen the contract calls out to the malicious `mooniswap` contract. This gives the malicious contract an easy opportunity to send some attacker-controlled token (ATTK) to the `ReferralFeeReceiver` in order to freely manipulate the frozen tokenbalances (tokens[0].uniBalanceOf(address(this)).sub(token0Balance);).\\nNote that the used token addresses are never stored anywhere. The balances recorded here are for an attacker-controlled token (ATTK), not the actual one that we're about to steal (e.g. DAI)\\nThe token balances are now set-up for checkout in the last step (claimFrozenEpoch).\\n```\\nfunction freezeEpoch(Mooniswap mooniswap) external validSpread(mooniswap) {\\n    TokenInfo storage token = tokenInfo[mooniswap];\\n    uint256 currentEpoch = token.currentEpoch;\\n    require(token.firstUnprocessedEpoch == currentEpoch, "Previous epoch is not finalized");\\n\\n    IERC20[] memory tokens = mooniswap.getTokens();\\n    uint256 token0Balance = tokens[0].uniBalanceOf(address(this));\\n    uint256 token1Balance = tokens[1].uniBalanceOf(address(this));\\n    mooniswap.withdraw(mooniswap.balanceOf(address(this)), new uint256[](0));\\n    token.epochBalance[currentEpoch].token0Balance = tokens[0].uniBalanceOf(address(this)).sub(token0Balance);\\n    token.epochBalance[currentEpoch].token1Balance = tokens[1].uniBalanceOf(address(this)).sub(token1Balance);\\n    token.currentEpoch = currentEpoch.add(1);\\n}\\n```\\n\\nA call to `claimFrozenEpoch` checks-out the previously frozen token balance.\\nThe `claim > 0` requirement was fulfilled in step 1.\\nThe token balance was prepared for the attacker-controlled token (ATTK) in step 2, but we're now checking out `DAI`.\\nWhen the contract calls out to the attackers `mooniswap` contract the call to `IERC20[] memory tokens = mooniswap.getTokens();` returns the address of the token to be stolen (e.g. DAI) instead of the attacker-controlled token (ATTK) that was used to set-up the balance records.\\nSubsequently, the valuable target tokens (DAI) are sent out to the caller in `_transferTokenShare`.\\n```\\nif (share > 0) {\\n    EpochBalance storage epochBalance = token.epochBalance[firstUnprocessedEpoch];\\n    uint256 totalSupply = epochBalance.totalSupply;\\n    user.share[mooniswap][firstUnprocessedEpoch] = 0;\\n    epochBalance.totalSupply = totalSupply.sub(share);\\n\\n    IERC20[] memory tokens = mooniswap.getTokens();\\n    epochBalance.token0Balance = \\_transferTokenShare(tokens[0], epochBalance.token0Balance, share, totalSupply);\\n    epochBalance.token1Balance = \\_transferTokenShare(tokens[1], epochBalance.token1Balance, share, totalSupply);\\n    epochBalance.inchBalance = \\_transferTokenShare(inchToken, epochBalance.inchBalance, share, totalSupply);\\n```\\n
Resolution\\nAccording to the client, this issue is addressed in 1inch-exchange/1inch-liquidity-protocol#2 and the reentrancy in `FeeReceiver` in 1inch-exchange/[email protected]e9c6a03\\n(This fix is as reported by the developer team, but has not been verified by Diligence).\\nEnforce that the user-provided `mooniswap` contract was actually deployed by the linked factory. Other contracts cannot be trusted. Consider implementing token sorting and de-duplication (tokenA!=tokenB) in the pool contract constructor as well. Consider employing a reentrancy guard to safeguard the contract from reentrancy attacks.\\nImprove testing. The methods mentioned here are not covered at all. Improve documentation and provide a specification that outlines how this contract is supposed to be used.\\nReview the “additional notes” provided with this issue.
```\\nif (path[0].isETH()) {\\n    tx.origin.transfer(availableBalance);  // solhint-disable-line avoid-tx-origin\\n} else {\\n    path[0].safeTransfer(address(mooniswap), availableBalance);\\n}\\n```\\n