The absence of an event or notification mechanism in case of an attempt to add an already existing bridge agent is an error in the contract's behavior.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/c0dc3550e0754571b82d7bfd8f0282ac8fa5e42f/src/BranchPort.sol#L319-L324

Vulnerability details

Impact

Lack of observability and potential confusion regarding the status of a bridge agent. Specifically:

Lack of Observability:

• Without an event being emitted when an attempt is made to add an already existing bridge agent, external systems or users might not be aware that the operation failed.
• This lack of observability could lead to confusion about whether the bridge agent was successfully added or not.

Potential Misunderstanding:

• If an external system or user assumes that the addition of the bridge agent was successful (without receiving an event indicating otherwise), they might proceed with operations based on that assumption.
• This could potentially lead to incorrect decisions or actions being taken, based on inaccurate information.
• If the bridge agent is expected to perform specific tasks based on its status (e.g., processing transactions), a misunderstanding about its status could lead to discrepancies in operations.

Proof of Concept

Function Signature and Modifier:

    function addBridgeAgent(address _bridgeAgent) external override requiresBridgeAgentFactory {

• The addBridgeAgent function takes an _bridgeAgent address as an argument.
• The external keyword indicates that the function can be called only externally (not from within the contract).
• override suggests that this function is overriding a function defined in an inherited contract.
• It has a modifier requiresBridgeAgentFactory, which is not shown in the provided code. It likely checks if the sender is a valid bridge agent factory.

Check if Bridge Agent Already Exists:

    if (isBridgeAgent[_bridgeAgent]) revert AlreadyAddedBridgeAgent();

• This line checks if the _bridgeAgent address already exists in the isBridgeAgent mapping (meaning it is already a registered bridge agent).
• If it does, it reverts the transaction with an error message AlreadyAddedBridgeAgent(), indicating that the bridge agent is already added.

Marking Bridge Agent as Active:

    isBridgeAgent[_bridgeAgent] = true;

• If the _bridgeAgent is not already registered, this line marks it as an active bridge agent by setting isBridgeAgent[_bridgeAgent] to true.
• This means the bridge agent is now recognized as valid.

Adding Bridge Agent to List:

    bridgeAgents.push(_bridgeAgent);

• This line appends the _bridgeAgent address to the bridgeAgents array.
• This array likely keeps track of all registered bridge agents.

You can see this function attempts to add a bridge agent. Before doing so, it checks if the provided bridge agent address is already registered. If it is, the function reverts. Otherwise, it marks the bridge agent as active and adds it to the list of registered bridge agents.

The fact that there's no mechanism to emit an event or notify external systems when an attempt is made to add an already existing bridge agent. This could lead to confusion or miscommunication about the status of the bridge agent.

addBridgeAgent

function addBridgeAgent(address _bridgeAgent) external override requiresBridgeAgentFactory {
    if (isBridgeAgent[_bridgeAgent]) revert AlreadyAddedBridgeAgent();

    isBridgeAgent[_bridgeAgent] = true;
    bridgeAgents.push(_bridgeAgent);
}

If a bridge agent is added more than once. The code currently checks if _bridgeAgent is already an existing bridge agent (isBridgeAgent[_bridgeAgent]). If it is, the function reverts with an error message AlreadyAddedBridgeAgent(), which implies that the bridge agent is already added.

However, there's no event or mechanism in place to emit a notification or record that an attempt was made to add an existing bridge agent. This means that external observers or systems interacting with the contract won't have a way of knowing if such an attempt was made.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding an event as I previously suggested:

event BridgeAgentAdded(address indexed bridgeAgent);

function addBridgeAgent(address _bridgeAgent) external override requiresBridgeAgentFactory {
    if (isBridgeAgent[_bridgeAgent]) revert AlreadyAddedBridgeAgent();

    isBridgeAgent[_bridgeAgent] = true;
    bridgeAgents.push(_bridgeAgent);

    emit BridgeAgentAdded(_bridgeAgent); // Add this line to emit the event
}

Now this event will notify external observers whenever a bridge agent is added, providing a clear record of such events.

Assessed type

Access Control

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[0xA5DF]

Emitting events is QA

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a