code-423n4 / 2024-01-decent-findings

3 stars 3 forks source link

Encoded `swapPayload` and `bridgePayload` bytes can contain arbitrary malicious calldata. #556

Closed c4-bot-8 closed 6 months ago

c4-bot-8 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L117-L124 https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L69-L72 https://github.com/code-423n4/2024-01-decent/blob/07ef78215e3d246d47a410651906287c6acec3ef/src/UTB.sol#L108-L124

Vulnerability details

Vulnerability Overview

There is currently no validation on the swap instructions or bridge instructions structs passed to these functions. So it is possible for a malicious user to encode arbitrary calldata in the swapPayload bytes which gets passed to and decoded by the target swappers.

Similarly, the additional bridgePayload sent across chains in bridgeAndExecute could contain arbitrary logic as well.

Potential exploits

So, with the current implementation, a user could pass arbitrary calldata for swaps or bridged executions, without actually transferring funds they have approval over. Adding validations and schema checks would prevent this exploit vector.

Proof of Concept

The lack of validation on external data from various user-supplied parameters: File:UTB.sol:swapAndExecute

function swapAndExecute(
    SwapAndExecuteInstructions calldata instructions,
    FeeStructure calldata fees,
    bytes calldata signature
)
    public
    payable
    retrieveAndCollectFees(fees, abi.encode(instructions, fees), signature)
{
    _swapAndExecute(
        instructions.swapInstructions,
        instructions.target,
        instructions.paymentOperator,
        instructions.payload,
        instructions.refund
    );
}

Precisely:

Exposure

This exposes the swapper and bridge integrations to several risks:

An:

Or similarly for arbitrary contract executions via bridges.

Affected Code Line 117-124, Line 69-72

// In UTB.sol
_swapAndExecute(
    instructions.swapInstructions,
    instructions.target,
    instructions.paymentOperator,
    instructions.payload,
    instructions.refund
);
}

// In various Swapper contracts 
SwapParams memory swapParams = abi.decode(
    swapInstructions.swapPayload,
    (SwapParams)
);

The swapAndExecute and bridgeAndExecute functions in UTB.sol are affected. These serve as the core pathways for users to perform cross-chain transactions via Decent.

Expected Behavior

These functions expect that the provided SwapInstructions and BridgeInstructions contain properly encoded information to integrate safely with target swappers and bridges registered in the system.

Root Cause Analysis

  1. There is no validation of the swapInstructions or bridgeInstructions parameters passed in by users.

  2. The encoded swapPayload and bridgePayload bytes can contain arbitrary malicious calldata.

  3. When passed unchecked data, downstream swappers/bridges decode using abi.decode and execute unintended logic.

  4. This can lead to loss of funds, stuck tokens, or execution of unintended logic.

Demonstration Scenario

Impact on End Users

End users of Decent trusting arbitrary frontends or encoded payloads are at risk of:

This undermines the safety and reliability of the system from an end user perspective.

Impact on Ecosystem

For teams building on top of Decent:

This ultimately impacts developer experience and trust in leverage Decent as a base layer.

Impact on Core Protocol

For the Decent core protocol:

Making the protocol more hardened before launch will improve security, velocity, and scalability.

Tools Used

Vs Code

Recommended Mitigation Steps

Assessed type

Invalid Validation

c4-pre-sort commented 6 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 6 months ago

raymondfam marked the issue as duplicate of #21

c4-judge commented 5 months ago

alex-ppg marked the issue as unsatisfactory: Invalid