Exploring Missed Vulnerabilities: Code4rena Maia DAO — Ulysses Audit Contest

I participated alongside 33audits and hexbyte for this audit. Although we only reported one Medium-Risk Vulnerability, we scored an A grade for both QA and the Analysis Report. My primary purpose for writing this blog is to learn from my peer auditors, understanding their methodologies and what led them to discover the vulnerability that we could not.

I will be focusing on the High/Medium findings and adding my own insights. I hope you learn a thing or two as well. So, let’s begin!

The Findings

[H-01] All tokens can be stolen from VirtualAccount due to missing access modifier

The Vulnerability: The payableCall function is lacking the necessary check for the modifier requiresApprovedCaller. As a result, anyone can invoke the function and deplete funds.

Learning: Although a relatively straightforward issue, I regret not detecting it. The fundamental methodology involves conducting thorough access control checks. Checking the solidity metrics should have promptly exposed this vulnerability.

[H-02] if the Virtual Account’s owner is a Contract Account (multisig wallet), attackers can gain control of the Virtual Accounts by gaining control of the same owner’s address in a different chain

The Vulnerability: The vulnerability emerges when a user sends signed messages from Branch to Port. If the address has not yet been assigned a Virtual Account, a new one is created based on the caller’s address. This issue is particularly pronounced when utilizing MultiSig Wallets, as they can have different addresses on various chains, unlike externally owned accounts (EOA).

An attacker can exploit the signed message to take control of Virtual Account creation and subsequently gain control over these accounts.

[H-03] Redeeming a Settlement won’t work for unsigned messages when the communicating dApps have different addresses on the different chains

The Vulnerability: The impact is that funds become unredeemable and are trapped in a settlement. The recipient Dapp on the BranchChain may possess a different address or might not even exist on the Root Chain. To address this issue, a solution would involve incorporating an argument that allows users to specify the refund recipient when creating settlements, without relying on a Virtual Account.

Learning: This has become a prevalent attack vector nowadays. Developers often assume that the addresses will be the same across all chains for a user, but this is not guaranteed, especially in the case of MultiSig Wallets. Be sure to add this to your checklist

[M-01] The governance will fail to add an ecosystem token if someone creates a hToken that uses that ecosystem token

The Vulnerability: The issue lies in the unrestricted ability for anyone to create an hToken, using the ecosystem token as the underlying asset, causing the mapping to be updated in setAddresses(). This vulnerability leads to a Denial of Service (DoS) scenario when the admin initiates it, leading to a revert.

Learning: This is a somewhat unique issue. My takeaway is to explore methods for disrupting admin-based functions, especially those with checks like if(value == address(0)), and proceed only under specific conditions.

[M-02] When using BaseBranchRouter as a router on the ‘Arbitrum’ branch, we are unable to invoke the ‘callOutAndBridge’ function.

The Vulnerability: The Arbitrum bridge does not utilize callOutAndBridge, as it functions as the root branch but is required to behave like local branches. When a user tries to make a deposit in other branches, the local hTokens are moved from the user to the local branch port and then burned. However, in Arbitrum, there is no distinction between local and global hTokens. As a result, they are sent directly to the root port.

[M-03] ArbitrumBranchBridgeAgent::_performFallbackCall function does not refund users their excess native gas deposit

The Vulnerability: _performFallbackCall fails to refund the tokens back to the sender in the case of Arbitrum, specifically, the refund of msg.value.

Learning: If a behavior or functionality is intended to work in a specific manner, ensure that it is applicable to all supported chains.

[M-04] addGlobalToken() localAdress could be overwritten

The Vulnerability: Due to concurrency, the second local token can overwrite the first local token. The issue arises when the first local token is still valid before the overwrite occurs. If someone exchanges the first local token and waits until after the overwrite, the initial local token becomes invalidated, resulting in the user losing the corresponding token.

Learning: While blockchain transactions are typically not affected by concurrency issues, the asynchronous nature of layer zero introduces the potential for overwriting problems. Understand the overall functioning of third-party libraries in general.

[M-05] No deposit cross-chain calls/communication can still originate from a removed branch bridge agent

The Vulnerability: Admin router contracts possess the capability to disable or toggle off any individual’s bridge agents for any reason using the removeBranchBridgeAgent() function in the CoreRootRouter.sol contract. While this action deactivates the branch bridge agent in the Branch chain’s BranchPort, it still permits “No Deposit” calls to be initiated from the deactivated branch bridge agent.

Learning:When a smart contract feature involves enabling or disabling, it is crucial to verify whether the functionality remains operational when in a disabled state.

[M-06] BaseBranchRouter._transferAndApproveToken may revert in some cases

The Vulnerability: The bytecode compiled by the solidity compiler from ERC20(_token).approve will check whether the return size is 1 byte. If not, revert will occur but incase of USDT token the return size is 0 byte. As a result, USDT approves always revert.

Learning: This discovery appears to be quite unique in my opinion. While there is a known issue related to approving USDT to zero first, encountering a similar situation for the first time.

[M-07] If RootBridgeAgent.lzReceiveNonBlocking reverts internally, the native token sent by relayer to RootBridgeAgent is left in RootBridgeAgent

The Vulnerability: This issue is highly specific to the LayerZero implementation and can be regarded as a standard concern. If an exception occurs within lzReceiveNonBlocking, lzReceive will not revert (except for the ARB branch). Consequently, the native token sent to RootBridgeAgent will remain in the RootBridgeAgent.

[M-09] Message channels can be blocked resulting in DoS

The Vulnerability: The issue arises from a lack of adherence to the standard integration best practices recommended by LayerZero.

Learning: When integrating with external third-party libraries, always make sure to thoroughly review their documentation for integration checklists. You are likely to discover potential issues and best practices. Special thanks to 33audits for this valuable tip.

[M-08] Depositors could lose all their deposited tokens (including the hTokens) if their address is blacklisted in one of all the deposited underlyingTokens

The Vulnerability: A fairly standard issue involving tokens from blacklisted users, such as USDT or USDC. The problem arises from the redemption process, which operates by sending back all the tokens deposited. Tokens can only be returned to the same address from which they were initially deposited, leading to the issue.

Learning: Always consider scenarios in which users or the protocol may be vulnerable to Denial-of-Service (DoS) attacks as a result of blacklisted users.

[M-10] Incorrect flag results to _hasFallbackToggled always set to false on createMultipleSettlement.

The Vulnerability: Users can specify if they want a fallback on their transaction, which prevents the transaction from reverting but due to an incorrect implementation, this would always be set to false.

Learning: Always keep an eye on logical errors. Don’t blindly trust what a function claims to do. Test it thoroughly, then verify the results.

[M-11] Incorrect source address decoding in RootBridgeAgent and BranchBridgeAgent’s _requiresEndpoint breaks LayerZero communication

The Vulnerability: This vulnerability effectively disrupts the entire branch-to-root and root-to-branch inter-chain communication. The issue is related to the encoding and decoding of addresses, which can lead to a Denial-of-Service (DoS) scenario.

Learning: Verify the proper functioning of the encode/decode path through thorough testing. Emphasize the issue related to incorrect array splits, indicating that a simple mistake was made by slicing the array at the wrong indexes.

[M-12] ArbitrumCoreBranchRouter.executeNoSettlement can’t handle 0x07 function

The Vulnerability: ArbitrumCoreBranchRouter extends CoreBranchRouter and overrides its executeNoSettlement function. While CoreBranchRouter.executeNoSettlement can handle the 0x07 function, the ArbitrumCoreBranchRouter.executeNoSettlement cannot.
I suspect this discrepancy arises from the fact that initially, CoreBranchRouter.executeNoSettlement did not handle the 0x07 function. Subsequently, when support for this function was introduced, ArbitrumCoreBranchRouter was not updated accordingly.

Learning: Comparing the code with previous audits or checking for new changes is always a good practice. Often, developers make the mistake of not implementing previous fixes correctly, inadvertently exposing potential issues.

Low/QA Issues

With respect to Low/QA reports, most of them originate from the fact that the developers did not adhere to the LayerZero integration checklist .

Maia DAO — Ulysses Contest Audit Report — https://code4rena.com/reports/2023-09-maia

Thank For Reading.

Connect with me: https://linktr.ee/zuhaib44