search

sherlock-audit / 2024-02-tapioca-judging

2 stars 2 forks source link

bin2chen - _internalTransferWithAllowance() users can use multi-signature wallets that do not belong to them under different chains. #99

Closed sherlock-admin3 closed 5 months ago

sherlock-admin3 commented 5 months ago

bin2chen

high

_internalTransferWithAllowance() users can use multi-signature wallets that do not belong to them under different chains.

Summary

Currently, many users possess multi-signature wallets. Due to the mechanism for generating multi-signature addresses the same address might not be controlled by the same user on different chains. However, the _internalTransferWithAllowance() function only verifies _owner != srcChainSender for allowance checks. As a result, users on Chain A can manipulate the same address's assets belonging to a different user's account on Chain B.

Vulnerability Detail

Several contracts currently rely on _internalTransferWithAllowance() to the allowance security check 

    function _internalTransferWithAllowance(address _owner, address srcChainSender, uint256 _amount) internal {
        if (_owner != srcChainSender) {
            _spendAllowance(_owner, srcChainSender, _amount);
        }

        _transfer(_owner, address(this), _amount);
    }

If from == srcChainSender, this method can be directly used. don't check allowance

Let's consider a scenario where the same allowance wallet address 0x123 belongs to Alice on Chain A but is belongs to Bob on Chain B.

In this case, Alice can execute usdo.lzCompose(_dstEid=B) to bypass the allowance security check and manipulate Bob's balance on Chain B.

There are multiple instances where allowance checks can be bypassed, including:

Here are examples from other protocol audits:

https://solodit.xyz/issues/h-02-if-the-virtual-accounts-owner-is-a-contract-account-multisig-wallet-attackers-can-gain-control-of-the-virtual-accounts-by-gaining-control-of-the-same-owners-address-in-a-different-chain-code4rena-maia-dao-maia-dao-git

Impact

Users can use the multi-signed wallet balance that does not belong to them under different chains.

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/usdo/modules/UsdoOptionReceiverModule.sol#L202

Tool used

Manual Review

Recommendation

It is recommended that if _owner is not EOA (code.length > 0), the same allowance check should be performed to ensure security.

    function _internalTransferWithAllowance(address _owner, address srcChainSender, uint256 _amount) internal {
-       if (_owner != srcChainSender) {
+       // check code for ensure the safety of multi-signature wallet cross-chain.
+       if (_owner != srcChainSender || _owner.code.length !=0) {
            _spendAllowance(_owner, srcChainSender, _amount);
        }

        _transfer(_owner, address(this), _amount);
    }

Duplicate of #138

cryptotechmaker commented 5 months ago

Low/Invalid; we allow EOAs and warn about the possible edge cases of using contracts. However, I think we can add the check

sherlock-admin3 commented 5 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

how would an address that alice owns on one chain have an owner on another chain, even in the report you made mentioned of there is a chance its near to impossible