Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smart Contract calling callOutSignedAndBridge via BranchBridgeAgent can cause loss of fund #872

Closed
c4-submissions opened this issue Oct 6, 2023 · 12 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-877 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchBridgeAgent.sol#L276
https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/RootBridgeAgent.sol#L577

Vulnerability details

Impact

Smart Contract calling callOutSignedAndBridge via BranchBridgeAgent can cause loss of fun

Proof of Concept

One of the cross-chain request pass is that when user calling callOutSignedAndBridge via BranchBridgeAgent

the payload is created

	//Encode Data for cross-chain call.
	bytes memory payload = abi.encodePacked(
		_hasFallbackToggled ? bytes1(0x85) : bytes1(0x05),
		msg.sender,
		_depositNonce,
		_dParams.hToken,
		_dParams.token,
		_dParams.amount,
		_dParams.deposit,
		_params
	);

this would trigger the code on RootBridgeAgent.sol

 } else if (_payload[0] & 0x7F == 0x05) {
            // Parse deposit nonce
            nonce = uint32(bytes4(_payload[PARAMS_START_SIGNED:PARAMS_TKN_START_SIGNED]));

            //Check if tx has already been executed
            if (executionState[_srcChainId][nonce] != STATUS_READY) {
                revert AlreadyExecutedTransaction();
            }

            // Get User Virtual Account
            VirtualAccount userAccount = IPort(localPortAddress).fetchVirtualAccount(
                address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED])))
            );

            // Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

            // Avoid stack too deep
            uint16 srcChainId = _srcChainId;

            // Try to execute remote request
            // Flag 5 - RootBridgeAgentExecutor(bridgeAgentExecutorAddress).executeSignedWithDeposit(address(userAccount), localRouterAddress, data, _srcChainId)
            _execute(
                _payload[0] == 0x85,
                nonce,
                address(uint160(bytes20(_payload[PARAMS_START:PARAMS_START_SIGNED]))),
                abi.encodeWithSelector(
                    RootBridgeAgentExecutor.executeSignedWithDeposit.selector,
                    address(userAccount),
                    localRouterAddress,
                    _payload,
                    srcChainId
                ),
                srcChainId
            );

            // Toggle Router Virtual Account use for tx execution
            IPort(localPortAddress).toggleVirtualAccountApproved(userAccount, localRouterAddress);

the msg.sender address in source chain (branch bridge agent chain) will be used to either fetch or create a new virtual wallet

   /// @inheritdoc IRootPort
    function fetchVirtualAccount(address _user) external override returns (VirtualAccount account) {
        account = getUserAccount[_user];
        if (address(account) == address(0)) account = addVirtualAccount(_user);
    }

and this function has no access control, anyone can trigger this function to create a virtual account for specific user address

and the function will not revert, if wallet does not exist, wallet is created for the user.

the code assume the same address in different blockchain belongs to the same owner

this is mostly true for EOA account

but not true for smart contract address (for example, multisig)

the same address for multisig in different network can belong to different owner

for example https://rekt.news/wintermute-rekt/

the false assumption of a mutlisig smart contract address is controlled by same owner in different network has cost 20M OP lost

the multisig address is controlled by wintermute in mainnet

the attacker observe the OP is sent to the same address in OP network

the attacker manage the get the factory nonce of the gensis safe factory and redeploy the same address in OP network to control the OP token

this could happens to the current implementation of maia dao

a user can observer when a multisig address trigger callOutSignedAndBridge and redeploy the same address in different network (root bridge agent) to control the fund

or it is possible a smart contract in blockchain A does not belong to anyone in blockchain B, in that case, the fund is lost

Tools Used

Manual Review

Recommended Mitigation Steps

let user specify the recipient when calling callOutSignedAndBridge and use the recipient address to fetch virtual account

Assessed type

Token-Transfer

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Oct 6, 2023
c4-submissions added a commit that referenced this issue Oct 6, 2023
@c4-pre-sort
Copy link

0xA5DF marked the issue as duplicate of #877

@c4-pre-sort c4-pre-sort added duplicate-877 sufficient quality report This report is of sufficient quality labels Oct 12, 2023
@c4-pre-sort
Copy link

0xA5DF marked the issue as sufficient quality report

@c4-judge
Copy link
Contributor

alcueca marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Oct 25, 2023
@c4-judge
Copy link
Contributor

alcueca changed the severity to 3 (High Risk)

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge duplicate-351 and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value duplicate-877 labels Oct 27, 2023
@c4-judge
Copy link
Contributor

alcueca marked the issue as duplicate of #351

@c4-judge
Copy link
Contributor

alcueca marked the issue as partial-50

@c4-judge c4-judge added partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed satisfactory satisfies C4 submission criteria; eligible for awards 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge labels Oct 27, 2023
@c4-judge
Copy link
Contributor

alcueca changed the severity to 2 (Med Risk)

@c4-judge c4-judge reopened this Oct 27, 2023
@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Oct 27, 2023
@c4-judge
Copy link
Contributor

alcueca marked the issue as selected for report

@c4-judge
Copy link
Contributor

alcueca marked the issue as not selected for report

@c4-judge c4-judge removed the selected for report This submission will be included/highlighted in the audit report label Oct 27, 2023
@c4-judge
Copy link
Contributor

alcueca marked issue #877 as primary and marked this issue as a duplicate of 877

@c4-judge c4-judge added duplicate-877 and removed primary issue Highest quality submission among a set of duplicates labels Oct 27, 2023
@c4-judge
Copy link
Contributor

alcueca marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue labels Oct 27, 2023
@c4-judge
Copy link
Contributor

alcueca changed the severity to 3 (High Risk)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-877 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

3 participants