hats-finance / Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d

Aleph Zero bridge to Ethereum
Apache License 2.0
0 stars 1 forks source link

Unsuccessful ERC-20 token transfer during relay to `ETH` will lead to user unable to access their funds #8

Open hats-bug-reporter[bot] opened 7 months ago

hats-bug-reporter[bot] commented 7 months ago

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

Description:

Impact

Short-term loss of funds if owner recovers ERC-20, otherwise permanent loss of funds if owner can't recover

Description

While sending ERC-20 from AlephZero to ETH: during receiveRequest() (if the signature count is sufficient) the ERC-20 transfer can fail but the transaction is not reverted: the requestHash is marked as processed and deleted from pendingRequests. This means these funds will be stuck in the contract and can only be recovered by the owner via recoverERC20().

Blacklisted tokens: It's worth to note that tokens with blacklist (USDT / USDC) are not supported by the current implementation. Sending to a blacklisted address is one instance where an ERC-20 transfer can fail.

Most.sol - receiveRequest()

    function receiveRequest(
        bytes32 _requestHash,
        uint256 _committeeId,
        bytes32 destTokenAddress,
        uint256 amount,
        bytes32 destReceiverAddress,
        uint256 _requestNonce
    ) external whenNotPaused _onlyCommitteeMember(_committeeId) {
        ...
        ...
        if (request.signatureCount >= signatureThreshold[_committeeId]) {
            processedRequests[requestHash] = true;
            delete pendingRequests[requestHash];

            address _destTokenAddress = bytes32ToAddress(destTokenAddress);
            address _destReceiverAddress = bytes32ToAddress(
                destReceiverAddress
            );
            // return the locked tokens
            if (_destTokenAddress == wethAddress) {
                (bool unwrapSuccess, ) = wethAddress.call(
                    abi.encodeCall(IWETH9.withdraw, (amount))
                );
                if (!unwrapSuccess) revert UnwrappingEth();
                (bool sendNativeEthSuccess, ) = _destReceiverAddress.call{
                    value: amount,
                    gas: GAS_LIMIT
                }("");
                if (!sendNativeEthSuccess) {
                    emit EthTransferFailed(requestHash);
                }
            } else {
                IERC20 token = IERC20(_destTokenAddress);
                if (
                    !tokenTransferReturnSuccess(
                        token,
                        _destReceiverAddress,
                        amount
                    )
                ) {
                    emit TokenTransferFailed(requestHash);
                }
            }
            emit RequestProcessed(requestHash);
        }
    }

Recommendation

Either revert the transaction when tokenTransferReturnSuccess returns false or immediately send back the token to the original sender on AlephZero via sendRequest().

krzysztofziobro commented 7 months ago

Out of scope section: "Loss/freezing of funds caused by user error: for example sending tokens to addresses that cannot receive funds/user has no access for."