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

Requests from BranchBridgeAgent will always revert #603

Closed
code423n4 opened this issue Jul 4, 2023 · 3 comments
Closed

Requests from BranchBridgeAgent will always revert #603

code423n4 opened this issue Jul 4, 2023 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-91 satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1006-L1011

Vulnerability details

Impact

_performCall function (in BranchBridgeAgent) whichs performs call to AnycallProxy Contract for cross-chain messaging has a wrong anycall flag. The flag is AnycallFlags.FLAG_ALLOW_FALLBACK which has value 4

    function _performCall(bytes memory _calldata) internal virtual {
        //Sends message to AnycallProxy
        IAnycallProxy(localAnyCallAddress).anyCall(
            rootBridgeAgentAddress, _calldata, rootChainId, AnycallFlags.FLAG_ALLOW_FALLBACK, ""
        );
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/BranchBridgeAgent.sol#L1006-L1011

FLAG_ALLOW_FALLBACK constant

   uint256 public constant FLAG_ALLOW_FALLBACK = 0x1 << 2;

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/lib/AnycallFlags.sol#L11

This flag is used to pay the fee on source chain. However, it should be the destination chain as it is clear from the code. check RootBridgeAgent.anyExecute method

    function anyExecute(bytes calldata data)
        external
        virtual
        requiresExecutor
        returns (bool success, bytes memory result)
    {
			.
			.
			.
			.
			//Zero out gas after use if remote call
			if (initialGas > 0) {
				_payExecutionGas(userFeeInfo.depositedGas, userFeeInfo.gasToBridgeOut, _initialGas, fromChainId);
			}
			.
	}

This causes the function to revert always since there is no deposit fee on source chain.

Please note that the sponsor also confirmed this.

Proof of Concept

According to AnyCall docs - parameter for fee:

4: Gas fee paid on source chain. Allow fallback
6: Gas fee paid on destination chain. Allow fallback

Here is a link to request-parameters for V7
https://docs.multichain.org/developer-guide/anycall-v7/how-to-integrate-anycall-v7#request-parameters

Since the AnyCall flag is 4, the fee is expected to be paid on the source chain, otherwise, the request will revert.

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace FLAG_ALLOW_FALLBACK with FLAG_ALLOW_FALLBACK_DST which has the value 6

    uint256 public constant FLAG_ALLOW_FALLBACK_DST = 6;

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/lib/AnycallFlags.sol#L12

Assessed type

Other

@code423n4 code423n4 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 Jul 4, 2023
code423n4 added a commit that referenced this issue Jul 4, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Jul 9, 2023

trust1995 marked the issue as duplicate of #91

@c4-judge c4-judge closed this as completed Jul 9, 2023
@c4-judge c4-judge added duplicate-91 satisfactory satisfies C4 submission criteria; eligible for awards labels Jul 9, 2023
@c4-judge
Copy link
Contributor

trust1995 marked the issue as satisfactory

@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 and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jul 11, 2023
@c4-judge
Copy link
Contributor

trust1995 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-91 satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

2 participants