8olidity - Malicious tokens can safeTransferFrom() results

[sherlock-admin]

8olidity

medium

Malicious tokens can safeTransferFrom() results

Summary

Malicious tokens can safeTransferFrom() results

Vulnerability Detail

In the CardTopupPermit() function, _token is passed in by the user and can be a malicious ERC20 token forged by the attacker

    function CardTopupPermit(address _token, uint256 _amount, bytes calldata _permit, uint256 _expectedMinimumReceived, bytes memory _convertData, uint256 _bridgeType, bytes memory _bridgeTxData, bytes32 _receiverHash) public payable {
        // this snippet is borrowed from 1inch contracts
        // but encodeWithSelector is replaced with encodePacked (how it worked in their code?) https://github.com/1inch/1inch-v2-contracts/blob/v2.1/contracts/OneInchExchange.sol#L173
        if (_permit.length == 32 * 7) {
            // solhint-disable-next-line avoid-low-level-calls
            (bool success, /*bytes memory result*/) = address(_token).call(abi.encodePacked(IERC20PermitUpgradeable.permit.selector, _permit));
            if (!success) {
                revert("permit call failed");
            }
        }

        // don't perform allowance check for native token (allowance is n/a in such case, it's provided by tx.value)
        if (_token != ETH_TOKEN_ADDRESS) {
            // we check 'current' allowance after executing permit (not the amount in permit itself)
            checkAllowance(_token, _amount);
        }

        // allowance checks/enforcing complete, perform actual topup (this flow is further unified across 3 possible topup public methods)
        _processTopup(msg.sender, _token, _amount, _expectedMinimumReceived, _convertData, _bridgeType, _bridgeTxData, _receiverHash);
    }

The _token is then put into the _processTopup function

    function _processTopup(address _beneficiary, address _token, uint256 _amount, uint256 _expectedMinimumReceived, bytes memory _convertData, uint256 _bridgeType, bytes memory _bridgeTxData, bytes32 _receiverHash) internal
    {
        // don't go further is contract function is paused (by admin or pauser)
        require(paused == false, "operations paused");

        // conversion is required, perform swap through exchangeProxy
        if (_token != ETH_TOKEN_ADDRESS) {
            // transfer tokens on the exchange proxy balance before performing swap call
            IERC20Upgradeable(_token).safeTransferFrom(_beneficiary, address(exchangeProxyContract), _amount);
        }

Here, a malicious _token can implement a fake safeTransferFrom function, such as no transfer but return a successful transfer. The contract also does not check the soundness of _token. Cause loss of contracted assets.

Impact

Malicious tokens can safeTransferFrom() results

Code Snippet

https://github.com/sherlock-audit/2022-10-mover/blob/main/cardtopup_contract/contracts/HardenedTopupProxy.sol#L331

    function CardTopupPermit(address _token, uint256 _amount, bytes calldata _permit, uint256 _expectedMinimumReceived, bytes memory _convertData, uint256 _bridgeType, bytes memory _bridgeTxData, bytes32 _receiverHash) public payable {
        // this snippet is borrowed from 1inch contracts
        // but encodeWithSelector is replaced with encodePacked (how it worked in their code?) https://github.com/1inch/1inch-v2-contracts/blob/v2.1/contracts/OneInchExchange.sol#L173
        if (_permit.length == 32 * 7) {
            // solhint-disable-next-line avoid-low-level-calls
            (bool success, /*bytes memory result*/) = address(_token).call(abi.encodePacked(IERC20PermitUpgradeable.permit.selector, _permit));
            if (!success) {
                revert("permit call failed");
            }
        }

        // don't perform allowance check for native token (allowance is n/a in such case, it's provided by tx.value)
        if (_token != ETH_TOKEN_ADDRESS) {
            // we check 'current' allowance after executing permit (not the amount in permit itself)
            checkAllowance(_token, _amount);
        }

        // allowance checks/enforcing complete, perform actual topup (this flow is further unified across 3 possible topup public methods)
        _processTopup(msg.sender, _token, _amount, _expectedMinimumReceived, _convertData, _bridgeType, _bridgeTxData, _receiverHash);
    }

Tool used

Manual Review

Recommendation

Example Set the token whitelist

[McMannaman]

The recommendation is significantly impacting the applicability of the contract and business flow.

Malicious tokens can safeTransferFrom() results

this lacks example of how this would allow to steal funds. Yes, user can do something with his/her own malicious token, but he could make the same play in e.g. Uniswap.

The contract also does not check the soundness of _token

Example of soundness check would be appreciated, otherwise it is of little practical value.