codehawks-bot commented 1 year ago

Fee on transfer tokens can cause lender tokens to get stuck in contract.

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L182

Summary

Fee on transfer tokens take a tax when they are submitted

Vulnerability Details

Some ERC20 tokens, such as USDT, allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance. However, a malicious user could also take advantage of this to steal funds from the pool.

Let's take a look at the addPool() and removePool() functions.

    /// @notice remove from the pool balance
    /// can only be called by the pool lender
    /// @param poolId the id of the pool to remove from
    /// @param amount the amount to remove
    function removeFromPool(bytes32 poolId, uint256 amount) external {
        if (pools[poolId].lender != msg.sender) revert Unauthorized();
        if (amount == 0) revert PoolConfig();
        _updatePoolBalance(poolId, pools[poolId].poolBalance - amount);
        // transfer the loan tokens from the contract to the lender
        IERC20(pools[poolId].loanToken).transfer(msg.sender, amount);
    }

    /// @notice add to the pool balance
    /// can only be called by the pool lender
    /// @param poolId the id of the pool to add to
    /// @param amount the amount to add
    function addToPool(bytes32 poolId, uint256 amount) external {
        if (pools[poolId].lender != msg.sender) revert Unauthorized();
        if (amount == 0) revert PoolConfig();

        _updatePoolBalance(poolId, pools[poolId].poolBalance + amount);
        // transfer the loan tokens from the lender to the contract
        IERC20(pools[poolId].loanToken).transferFrom(
            msg.sender,
            address(this),
            amount
        );
    }

Impact

addToPool() takes in an amount that is set by the users. removeFromPool() does the same, however when attempting to remove a pool using fee on transfer tokens the function would revert due to insufficient balance leaving the users funds stuck in the pool. Consider the example below.

Step to Reproduce

Alice sends 100 of FEE tokens to the contract when calling addToPool().
FEE token contract takes 10% of the tokens (10 FEE).
90 FEE tokens actually get deposited in contract.
_updatePoolBalance(poolId, pools[poolId].poolBalance + amount); will equal 100.
Attacker then sends 100 FEE tokens to the contract
The contract now has 180 FEE tokens but each user has an accounting of 100 FEE.
The attacker then tries to redeem his collateral for the full amount 100 FEE tokens.
The contract will transfer 100 FEE tokens to Bob taking 10 of Alices tokens with him.
Bob can then deposit back into the pool and repeat this until he drains all of Alice's funds.
When Alice attempts to withdraw the transaction will revert due to insufficient funds.

Tools used

Manual Review

Recommended Steps

Measure the contract balance before and after the call to transfer()/transferFrom() and use the difference between the two as the amount, rather than the amount stated

   function addToPool(bytes32 poolId, uint256 amount) external {
        if (pools[poolId].lender != msg.sender) revert Unauthorized();
        if (amount == 0) revert PoolConfig();
        uint256 balanceBefore = IERC20(pools[poolId].loanToken).balanceOf(address(this))

        IERC20(pools[poolId].loanToken).transferFrom(
               msg.sender,
               address(this),
               amount
           );
        uint256 balanceAfter = IERC20(pools[poolId].loanToken).balanceOf(address(this))
        uint256 _amount = balanceBefore - balanceAfter
        _updatePoolBalance(poolId, pools[poolId].poolBalance + _amount);
        // transfer the loan tokens from the lender to the contract  
    }

Note this implementation is vulnerable to reentrancy so use a nonreentrant guard in on this function.

leeftk commented 1 year ago

This report was marked as duplicate however it is actually different from the other one I submitted which got marked as the selected report. Whether you want to payout for an issue this similar or not is up to you (though I think you should as c4 does :) ) it is still crucial that the protocol be aware that this issue exists. The other fee on transfer report mentions the issue with fee on transfer in the Staking.sol contract however does not mention the issue in Lending.sol which is why I find it important that the protocol team be made aware of this. My original thought was the same issue affecting to separate parts of the protocol should be submitted as two separate issues, since this wasn't made clear in the docs I assumed this.

My suggestion is that if it is not going to be counted as two separate issues to mention in the docs the same bug affecting different parts of the codebase will be considered one issue. I believe Sherlock handles issues in this way however c4 tends to do the opposite. If you have a bug that has multiple attack vectors I've seen them submitted as separate issues before on c4. Either way you want to go makes sense to me, however, it should be made clear in the docs so that people don't submit multiple issues. :)

leeftk commented 1 year ago

Here is an example on c4 of where fee on transfer tokens effected two different contracts in the same contest and they were counted as two separate issues.

https://solodit.xyz/issues/m-15-bonding-doesnt-work-with-fee-on-transfer-tokens-code4rena-malt-finance-malt-finance-contest-git

https://solodit.xyz/issues/m-17-auction-collateraltoken-wont-work-if-token-is-fee-on-transfer-token-code4rena-malt-finance-malt-finance-contest-git

kosedogus commented 1 year ago

This report was marked as duplicate however it is actually different from the other one I submitted which got marked as the selected report. Whether you want to payout for an issue this similar or not is up to you (though I think you should as c4 does :) ) it is still crucial that the protocol be aware that this issue exists. The other fee on transfer report mentions the issue with fee on transfer in the Staking.sol contract however does not mention the issue in Lending.sol which is why I find it important that the protocol team be made aware of this. My original thought was the same issue affecting to separate parts of the protocol should be submitted as two separate issues, since this wasn't made clear in the docs I assumed this.

My suggestion is that if it is not going to be counted as two separate issues to mention in the docs the same bug affecting different parts of the codebase will be considered one issue. I believe Sherlock handles issues in this way however c4 tends to do the opposite. If you have a bug that has multiple attack vectors I've seen them submitted as separate issues before on c4. Either way you want to go makes sense to me, however, it should be made clear in the docs so that people don't submit multiple issues. :)

It might be because the contest mentioned is happened in 2021, so many things changed since then I believe and I didn't see token compatibility related issues counted as seperate for seperate contracts lately. Furthermore there are 50 duplicates for fot tokens and nearly half of them is actually mentioning problems regarding whole protocol, not just one contract. If judge decided to seperate the issues (which I don't recommend), judge needs to check all of them and seperate accordingly (This comment is for judge, not you).

kosedogus commented 1 year ago

And also if this issue get separated, then all other token issues must also be separated. For example decimal issues are all mentioning different part of codebase. But again the issue here is incompatibility of protocol with certain token types. So I believe separation has no point.

PatrickAlphaC commented 1 year ago

@leeftk I think the fee-on-transfer issue has effects all over the contracts. For now, I'm going to keep them as separate issues. The protocol is aware of the issue and how it has ramifications many places.

Cyfrin / 2023-07-beedle