Weird ERC20 tokens that are used as rewards from Votium will get stuck in the `VotiumStrategy` contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L215-L220 https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L280-L291

Vulnerability details

Bug Description

In VotiumStrategyCore.sol, the withdrawStuckTokens() function is used by the owner to withdraw ERC20 tokens from the Votium contract that are not distributed as reward:

VotiumStrategyCore.sol#L215-L220

    function withdrawStuckTokens(address _token) public onlyOwner {
        IERC20(_token).transfer(
            msg.sender,
            IERC20(_token).balanceOf(address(this))
        );
    }

As seen from above, it uses Openzeppelin's IERC20 interface to call transfer():

IERC20.sol#L41

    function transfer(address to, uint256 amount) external returns (bool);

However, as the interface expects transfer() to return a bool, withdrawStuckTokens() cannot be used to withdraw ERC20 tokens that do not return a bool on transfer(). Some examples would include USDT and BNB.

When transfer() is called, Solidity will attempt to decode the data returned by transfer() into a bool due to its interface's definition. However, since these tokens do not have any return value, the data returned will be empty, causing withdrawStuckTokens() to revert when attempting to decode its return data.

The applyRewards() function is used to sell ERC20 tokens from the Votium contract for ETH and distribute them as rewards:

VotiumStrategyCore.sol#L280-L291

            if (allowance != type(uint256).max) {
                if (allowance > 0) {
                    IERC20(_swapsData[i].sellToken).approve(
                        address(_swapsData[i].spender),
                        0
                    );
                }
                IERC20(_swapsData[i].sellToken).approve(
                    address(_swapsData[i].spender),
                    type(uint256).max
                );
            }

As seen from above, if the spender's allowance is not uint256 max, it calls approve() to set the spender's allowance to 0 before approving it to uint256 max.

However, such an implementation will not work for ERC20 tokens that revert on zero value approvals, such as BNB. If applyRewards() is called for such a token, the first call to approve() would revert, causing applyRewards() to revert.

Impact

withdrawStuckTokens() and applyRewards() are not be callable for certain ERC20 tokens that do not adhere to the ERC20 standard, which could cause them to become permanently stuck in the VotiumStrategy contract. This includes widely used tokens such as USDT and BNB.

Note that the likelihood of such a token being used as rewards from Votium is not low according to the contest's README:

For rewards there could be theoretically any ERC20's that come in as rewards from Votium

Recommended Mitigation

Consider using Openzeppelin's SafeERC20 library to handle token transfers and approvals.

Note to judge

I am aware that "Unsafe ERC20 Operations" has been flagged out in the bot report under L-3. However, I have decided to raise it to medium severity, given that Votium might eventually use a weird ERC20 token for rewards in the future, and it will be permanently stuck in the VotiumStrategy contract.

Note that the C4 docs explicitly states that raising issues from bot reports to a higher severity is fair game, as seen here:

Wardens may use automated tools as a first pass, and build on these findings to identify High and Medium severity issues ("HM issues"). However, submissions based on automated tools will have a higher burden of proof for demonstrating to sponsors a relevant HM exploit path in order to be considered satisfactory.

Assessed type

Token-Transfer

[elmutt]

https://github.com/asymmetryfinance/afeth/pull/163

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[romeroadrian]

@0xleastwood I've covered these cases in L-13 and L-14 of my QA report

• https://github.com/code-423n4/2023-09-asymmetry-findings/blob/main/data/adriro-Q.md#l-13-missing-usage-of-safe-wrappers-to-handle-erc20-operations
• https://github.com/code-423n4/2023-09-asymmetry-findings/blob/main/data/adriro-Q.md#l-14-zero-token-allowance-can-cause-denial-of-service-in-applyrewards

Note that the ERC20 transfer issue is also already covered by the automated findings https://gist.github.com/romeroadrian/a2045e828aa87418a66e9ab47d811292

[0xleastwood]

Honestly, I interpreted this issue as more than just L-13 and L-14. I thought there was something also being outlined about the onlyRewarder role making arbitrary approvals which can be used to rug reward tokens whenever. I guess that's not the case so glad you raised this. Will downgrade to QA.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)