Reward token can be unclaimable and stuck in an edge case

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x9d69fab0be3135cef24731650fbe7948fe972ec798a5bdf4e414b4dabc6aea3a Severity: low

Description:

Impact

Reward token can't be claimed in an edge case when reward token matches the principal token.

Description

It's possible that one of the reward tokens from Compounder such as USDC is the same as the principal token. The problem is that there will be no way to transfer out the reward token in this case, since convert() will revert if the arbitrary _token value is the principal token. HMX can also decide to change the reward tokens paid out in the future.

contracts/Portal.sol - convert()

    function convert(address _token, uint256 _minReceived, uint256 _deadline) external nonReentrant {
        ...
        if(_token == PRINCIPAL_TOKEN_ADDRESS) {revert InvalidToken();}
        ...

Another problem is that while calling stake(): _depositToYieldSource() will re-deposit the principal / reward token to HLP_STAKING -> even if convert() did not revert it's likely there won't be a profitable amount of reward token in the contract to call convert() if the main reward token is the principal token.

contracts/Portal.sol - _depositToYieldSource()

    function _depositToYieldSource() private {
        /// @dev Read how many principalTokens are in the contract and approve this amount
        uint256 balance = IERC20(PRINCIPAL_TOKEN_ADDRESS).balanceOf(address(this));
        IERC20(PRINCIPAL_TOKEN_ADDRESS).approve(HLP_STAKING, balance);

        /// @dev Transfer the approved balance to the external protocol using the interface
        IStaking(HLP_STAKING).deposit(address(this), balance);

        /// @dev Emit an event that tokens have been staked for the user
        emit TokenStaked(msg.sender, balance);
    }

I reckon this is not a likely outcome, however the impact would be high if such portal is deployed: e.g. a USDC Portal would be vulnerable.

Recommendation

Make sure to verify before future deployments that none of the reward tokens from Compounder is the same as the principal token. Optionally for extra safety: consider to initialize the expected reward tokens as an array storage variable and revert the deployment if one of them matches _PRINCIPAL_TOKEN_ADDRESS in the constructor.

[PossumLabsCrypto]

This is an interesting edge case that I haven´t thought about yet, thanks!

If HLP becomes the reward token for HLP staking - which is far fetched but not impossible - then the Portal would compound HLP on every stake() call as the whole balance is transferred to the compounder of the HMX protocol. Because these transferred rewards are not attributed to a user´s stake, they will be permanently stuck in the compounder of the HMX protocol.

Also, convert() would almost become useless because it cannot convert HLP. However, if the reward token changes to any other ERC20 token, it would not be a problem because convert() can target any token inside the Portal.

I´m aware that this logic must potentially be changed for other staking possibilities in the future but it even poses a far outside risk for the currently planned HLP.

An appropriate fix could be the following.

1. In convert() remove if(_token == PRINCIPAL_TOKEN_ADDRESS) {revert InvalidToken();}

2. Change the _depositToYield() function to this: function _depositToYieldSource(uint256 _amount) private { /// @dev approve the amount to be transferred to the external protocol IERC20(PRINCIPAL_TOKEN_ADDRESS).approve(HLP_STAKING, 0); IERC20(PRINCIPAL_TOKEN_ADDRESS).approve(HLP_STAKING, _amount);

/// @dev Transfer the approved balance to the external protocol using the interface IStaking(HLP_STAKING).deposit(address(this), _amount);

/// @dev Emit an event that tokens have been staked for the user emit TokenStaked(msg.sender, _amount); }

3. Change this part in the stake() function: ......... /// @dev Deposit the principal into the yield source (external protocol) _depositToYieldSource(_amount); .........

Now, the Portal only stakes as much HLP into the compounder as the user deposits while remaining HLP stays in the contract and can be arbitraged via convert().

[PossumLabsCrypto]

Talking this through, I think there could be a problem if the reward token is changed to native ETH instead of an ERC20 token as this cannot be targeted by convert().

Any thoughts on that?

[0xfuje]

Thank you!

That is true, Portal would compound HLP on every stake() and transfer it to HMX compounder. If HLP are claimed in one of the claim reward funcs, it will be transferred back to the compounder as soon as someone stakes. An attacker could also backrun claimRewards() with a 1 wei stake() to send back the HLP to compounder. Yeah if the transferred reward amount would be the exact amount sent by the user this wouldn't be a problem.

Yes, convert() would most likely be useless. I don't see how enough HLP could be accumulated as reward token to be profitable to call convert() in this case. Any other reward token would be fine.

Proposed fix seems like a really good way to solve this. I will think more about it to make sure it's secure.

I think medium severity might be more appropriate for this since it poses potential risk to the current Portal as well. Do you agree or disagree?

---

That's a possibility, even if unlikely. I think convert() could support native ETH as well with a little added complexity in the form of an isNative boolean:

    function convert(address _token, uint256 _minReceived, uint256 _deadline, bool isNative) external nonReentrant {
        /// @dev Require that the output token is a valid address and not the input or stake token (PSM / HLP)
        if (!isNative) {
            if(_token == PSM_ADDRESS) {revert InvalidToken();}
            if(_token == PRINCIPAL_TOKEN_ADDRESS) {revert InvalidToken();}
            if(_token == address(0)) {revert InvalidToken();}
        }

        /// @dev Require that the deadline has not expired
        if(_deadline < block.timestamp) {revert DeadlineExpired();}

        /// @dev Check if sufficient output token is available in the contract for frontrun protection
        uint256 contractBalance;
        if (isNative) {
            contractBalance = address(this).balance;
        } else {
            contractBalance = IERC20(_token).balanceOf(address(this));
        }

        if(contractBalance < _minReceived) {revert InvalidOutput();}
        if(contractBalance == 0) {revert InvalidOutput();}

        /// @dev Transfer the input (PSM) token from the user to the contract
        IERC20(PSM_ADDRESS).safeTransferFrom(msg.sender, address(this), AMOUNT_TO_CONVERT); 

        /// @dev Update the funding reward pool balance and the tracker of collected rewards
        if (bToken.totalSupply() > 0 && fundingRewardsCollected < fundingMaxRewards) {
            uint256 newRewards = (FUNDING_REWARD_SHARE * AMOUNT_TO_CONVERT) / 100;
            fundingRewardPool += newRewards;
            fundingRewardsCollected += newRewards;
        }

        /// @dev Transfer the output token from the contract to the user
        if (isNative) {
            (bool sent, ) = payable(msg.sender).call{value: contractBalance}("");
            if (!sent) {revert FailedToSendNativeToken();}
        } else {
            IERC20(_token).safeTransfer(msg.sender, contractBalance);
        }
    }

[PossumLabsCrypto]

I agree that medium is the right category here. Thanks for your help also in the related issue with native ETH! 👍

What do you think about these adjustments to your proposed solutions? I try to optimize for lowest number of inputs and LOC to keep the code compact. (leave out the boolean and instead use Adress(0) input as indication for native ETH handling)

    function convert(address _token, uint256 _minReceived, uint256 _deadline) external nonReentrant {
        /// @dev Require that the output token is not the input token (PSM)
        if(_token == PSM_ADDRESS) {revert InvalidToken();}

        /// @dev Require that the deadline has not expired
        if(_deadline < block.timestamp) {revert DeadlineExpired();}

        /// @dev Get the contract balance of the correct token
        uint256 contractBalance;
        if(_token == address(0)) {
           contractBalance = address(this).balance;         
        } else {
           contractBalance = IERC20(_token).balanceOf(address(this));
        }

        /// @dev Check if sufficient output token is available in the contract for frontrun protection
        if(contractBalance < _minReceived) {revert InvalidOutput();}
        if(contractBalance == 0) {revert InvalidOutput();}

        /// @dev Update the funding reward pool balance and the tracker of collected rewards
        if (bToken.totalSupply() > 0 && fundingRewardsCollected < fundingMaxRewards) {
            uint256 newRewards = (FUNDING_REWARD_SHARE * AMOUNT_TO_CONVERT) / 100;
            fundingRewardPool += newRewards;
            fundingRewardsCollected += newRewards;
        }

        /// @dev Transfer the input token (PSM) from the user to the contract
        IERC20(PSM_ADDRESS).safeTransferFrom(msg.sender, address(this), AMOUNT_TO_CONVERT); 

        /// @dev Transfer the output token from the contract to the user
        if(_token == address(0)) {
            payable(msg.sender).transfer(contractBalance);
        } else {
            IERC20(_token).safeTransfer(msg.sender, contractBalance);
        }
    }

[0xfuje]

Thank you, happy to help :)

Understandable you optimize for low LOC. Looks good to me! Only thing I would change is the native transfer: call is safer to use since transfer and send only forwards 2300 gas to use, which could potentially break if an opcode's gas cost increases or a contract's fallback uses more than 2300 gas.

[0xfuje]

Excuse me for missing how the contract receives ETH, I only did think about the mitigation in the context of convert. As rightfully pointed out by #69 -> receive() function should be added (if you choose to support native ETH).