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

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

Eth Lockup Risk in Most Contract's receiveRequest() Function #60

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

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

Github username: @0xLuckyLuke Twitter username: 0xLuckyLuke Submission hash (on-chain): 0xce2989fb05c3aa1dd6a4c4d1822d7cfa00495926495d4578bcf4495a6dc318de Severity: low

Description: Description\ During the execution of the Most::receiveRequest() function in the Most contract, if the signature count meets the threshold and the Ethereum (ETH) transfer fails, the transaction proceeds without reverting. As a result, the requestHash associated with the failed transfer is marked as processed and removed from the pendingRequests mapping. Consequently, the funds intended for transfer become trapped within the contract and can only be retrieved by the owner through the recoverNative() function.

// 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);
                }
            }

Impact\ Short-term loss of funds (Owner can use recoverNative() function)

Recommendation\ To mitigate this vulnerability, consider revising the logic in the receiveRequest() function. If the sendNativeEthSuccess flag returns false, indicating a failed ETH transfer, the transaction should be reverted to prevent marking the request as processed prematurely. Alternatively, an immediate fallback mechanism could be implemented to return the tokens to the original sender on AlephZero, ensuring the proper handling of failed transactions and preventing funds from becoming trapped within the contract.

krzysztofziobro commented 4 months ago

No PoC showcasing the issue. If the failure is meant to be caused by a revert from receiver contract, then see 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."