Skip to content

Latest commit

 

History

History
1209 lines (978 loc) · 54.7 KB

2024-12-soon.md

File metadata and controls

1209 lines (978 loc) · 54.7 KB

Soon Contest

Soon Protocol contest || SVM, layer2, OP Stack || 23 Dec 2024 to 20 Jan 2025 on Cantina

My Finding Summay

ID Title Severity
H-01 Depositing ETH uses _value instead of _mint HIGH
H-02 All Transactions will get directed to an incorrect L2 receiver HIGH
H-03 Retrieving Selector function can result in the Panic of the thread HIGH
H-04 Attackers can Drain L2 Bridge because of not authorizing L1Sender or L2Receiver HIGH
M-01 Users overpay fees when Bridging ETH/ERC20 tokens MEDIUM
M-02 Failure Parsed Deposit Transactions Logs, or sanitizing Skip all deposit transactions MEDIUM
M-03 All Bridged ERC721 tokens will get Lost MEDIUM
M-04 There is no recovery mechanism for reverted Deposit Transactions MEDIUM
M-05 Users can generate yields at the expense of the Bridge by depositing small amounts MEDIUM
M-06 Renting check suppose All accounts with 0 length MEDIUM
M-07 from address should get DeAllaised if the Call is Bridging tokens MEDIUM
M-08 uint64 is too small For 18 decimal Tokens deposited in Soon MEDIUM
M-09 DepositERC20 Is not enforcing success MEDIUM
M-10 Soon is not handling Direct deposits from Portal MEDIUM
L-01 Creating Contracts L1 -> L2 is Not Supported in Soon LOW
L-02 Soon Nodes do not decode extraData after extracting it. LOW
L-03 The depositing value in Soon is uint128 and uint256 in Ethereum LOW
I-01 Soon Bridge is not Processing Relayer messages INFO
I-02 Gas value is set by users in L1 and not used in L2 INFO

[H-01] Depositing ETH uses _value instead of _mint

Description

When Bridging ETH from L1 to L2, Optimism represents the value deposited in L1 in _mint and the value to be received by L2 in _value.

    /// @notice Common logic for creating deposit transactions.
    /// @param _to         Target address on L2.
>>  /// @param _mint       Units of asset to deposit into L2.
>>  /// @param _value      Units of asset to send on L2 to the recipient.
    /// @param _gasLimit   Amount of L2 gas to purchase by burning gas on L1.
    /// @param _isCreation Whether or not the transaction is a contract creation.
    /// @param _data       Data to trigger the recipient with.
    function _depositTransaction(
        bytes32 _to,
>>      uint256 _mint,
>>      uint256 _value,
        uint64 _gasLimit,
        bool _isCreation,
        bytes memory _data
    ) ... {
        ...
        // Emit a TransactionDeposited event so that the rollup node can derive a deposit
        // transaction for this deposit.
        emit TransactionDeposited(from, _to, DEPOSIT_VERSION, opaqueData);
    }

In Optimism We can directly call _depositTransaction() through Portal, and value can be different from mint. This seems to be weird from the first Spot, but we need to understand What Optimism Node do in case mint is different than value.

mint is the amount sent by the L1User to Portal, in case value is greater than teh mint value, Optimism Nodes take the rest of the assets from the L2 sender address.

This is OK in EVM as the sender who caller the message in L1, is an authorized sender with EOA, making him accepts that action. SO For example in Optimism:

  • UserA caller depositTransaction directly
  • mint: 5 ETH
  • value: 10 ETH
  • Optimism Node will take 10 - 5 = 5 ETH from the from address in L2 and transfer it to to address in L2
  • This is like a double transaction in 1
    • Bridging 5 ETH from L1From to L2to
    • Transferring 5 ETH from L2From to L2to

This behavior can be made as L1 and L2 in Optimism as both of them is EVM. but in Soon L2 is SVM.

This process is not even implemented in Soon nodes, so we should use mint value, which represent msg.value / Rate instead of the value as there is no mechanism for taking value from From in L2 as addresses in L2s are bytes32 and Address in L1.

In L2Bridge::deposit_eth, we use the value to transfer it to the receiver, instead of mint.

    fn deposit_eth(
        program_id: &Pubkey,
        accounts: &[AccountInfo],
        _from: Address,
        _to: Address,
        _mint: u128,
>>      value: u128,
        ...
    ) -> ProgramResult {
        ...
        let min_balance = Rent::get()?.minimum_balance(0);
>>      let mut transfer_value = value as u64; // @audit-high This should be `_mint`

        ...
    }

This Will allow Deposits from Direct Optimism Portal to make mint smaller than value, allowing them to drain Bridge ETH Liquidity.

Recommendations

In OptimismPortal::_depositTransaction(), enforce mint value to equal value passed.

[H-02] All Transactions will get directed to an incorrect L2 receiver

Description

When parsing Logs from TransactionDepsoited() event for L1 -> L2 transaction. from address is an EVM address and to address should be bytes32 address to be compatible with Solana.

>>  event TransactionDeposited(address indexed from, bytes32 indexed to, uint256 indexed version, bytes opaqueData);
    ...
    function _depositTransaction( ... ) ... {
      ...
>>    emit TransactionDeposited(from, _to, DEPOSIT_VERSION, opaqueData);
    }

to is of type bytes32, as all Solana PubKeys are 32 bytes in Length, but when retreiving the to address, we wrap it to EVM address instead of SVM address

impl TryFrom<Log> for UserDeposited {
    type Error = DepositError;

    /// Converts the emitted L1 deposit event log into [UserDeposited]
    fn try_from(log: Log) -> Result<Self, Self::Error> {
        let opaque_data = log.data().data.to_vec();

        let from = Address::from_word(log.topics()[1]);
>>      let to = Address::from_word(log.topics()[2]);
        let (ext, data) = ExternalData::extract(&opaque_data)?;
        let deposit_type = DepositType::try_from(data)?;

        ...
    }
}

to address is labeled as Address instead of Pubkey, which will make all destination addresses being totally incorrect, leading to totally incorrect L2 -> L1 executions.

Recommendations

use Pubkey instead of Address for the to address.

[H-03] Retrieving Selector function can result in the Panic of the thread

Description

After L2 Gets all Logs from L1::Portal TransactionDeposited, we retrive the information for the opaqueData field.

node/primitives/src/desposit.rs - impl TryFrom for UserDeposited

    /// Converts the emitted L1 deposit event log into [UserDeposited]
    fn try_from(log: Log) -> Result<Self, Self::Error> {
1:      let opaque_data = log.data().data.to_vec();

        let from = Address::from_word(log.topics()[1]);
        let to = Address::from_word(log.topics()[2]);
2:      let (ext, data) = ExternalData::extract(&opaque_data)?;
        let deposit_type = DepositType::try_from(data)?;

        ...
    }
// --------------
    pub fn extract(opaque_data: &[u8]) -> Result<(Self, Vec<u8>), DepositError> {
        let mint = U256::from_be_slice(&opaque_data[0..32]);
        let value = U256::from_be_slice(&opaque_data[32..64]);
        let gas = u64::from_be_bytes(opaque_data[64..72].try_into().map_err(
            |e: TryFromSliceError| {
                DepositError::ParseDepositTxError(format!("convert gas limit failed: {e}"))
            },
        )?);
        let is_creation = opaque_data[72] != 0;

3:      let rest_start = Self::selector_end_index(opaque_data)?;
❌️      let extra_data = opaque_data[73..rest_start].to_vec();
        let rest = opaque_data[rest_start..].to_vec();

        ...
    }
// ---------------
    const ETH_DEPOSIT_SELECTOR: [u8; 4] = hex!("596a37c5");
    const ERC20_DEPOSIT_SELECTOR: [u8; 4] = hex!("f73fb39c");

    fn selector_end_index(opaque_data: &[u8]) -> Result<usize, DepositError> {
        for i in 0..opaque_data.len() {
            if i + 4 <= opaque_data.len() {
4:              if opaque_data[i..i + 4] == ETH_DEPOSIT_SELECTOR
                    || opaque_data[i..i + 4] == ERC20_DEPOSIT_SELECTOR
                {
5:                  return Ok(i);
                }
            } else {
                break;
            }
        }
        Err(DepositError::ParseDepositTxError(
            "selector not found".to_string(),
        ))
    }

The first thing is to retrieve the data object from the Event [1], since our TransactionDeposited contains opaqueData field, we will get this data to extract values.

The opaqueData contains the following.

src/L1/OptimismPortal::_depositTransaction

        bytes memory opaqueData = abi.encodePacked(_mint.ETHToRD(), _value.ETHToRD(), _gasLimit, _isCreation, _data);

mint, value, gas limit, is Creation and data. The data is set by the user but in case of Bridging Tokens, this field contains, the Selectors for Bridge Transaction and relayer, where the messages are encoded and send as we know in normal Optimism.

We extract data in [2], and for knowing we perform a check to know weather there is a Bridge of tokens or not in [3] by calling selector_end_index.

The issue here is that we pass all the data to be checked and the check process is done by checking an iterative 4 bytes in [4] and once they match they return the position of the selector [5].

The poblem is that there are previous values mint, value, gas limit, as will as Relayer Data which comes first in the data field, that encapsulates the bridge data, so in case the value existed earlier in gasLimit for example, the selector will be taken totally in the incorrect position.

Proof of Concept

  • userA wants to Bridge ERC20 tokens
  • gasLimit is set to 1500133317 in decimals, which is 0x596a37c5 in Hexa (ETH selector)
  • The Loop will return the index of gasLimit instead of the index of the encoded data of bridging ETH function

The problem is that when retrieving data, we are making assumptions that the position of the selector is after the element 72, but in this example, the position, where we slice the Data starts from 73.

        let extra_data = opaque_data[73..rest_start].to_vec();

But in such a case, the position will be 64 so this will result in Panic, because of out of Bound Array Access.

Panic will not get cached by the processor (it is not an Error), and will terminate the process directly.

Impacts

  • Panic the process will terminate all deposits being processed affecting all other deposits in that Batch
  • DoS as processing this deposit will always panic the process, resulting in halting the BlockChain L2 -> L1 module
  • UserA will not receive his Funds.
  • Attacker can use this method by simply providing a gasLimit with such a value in ETH or ERC20 or a combination of data from mint and value resulting in matching selector, and call it more than one time, to halt the BlockChain.

Note: Another issue that can occur is incorrect parsing of data, as still in case the index comes after 73 this doesn't mean that we catch the right position as in Optimism The L1 Bridge encodes The message with the selector first, then pass it as message to the relayer, and relayer takes the message and encapsulates it to data, putting the message in the Last, so We still go through The Relayer selector and the data for the Relayer, which still can make a collision in Selector before its right position.

Recommendations

In the case of bridging tokens/ETH, we encode Bridge Selector and make message then take the message and put it in the L1 Cross Messanger to get data then this data is getting put in data field in OpaqueData.

We need to skip the Opaque Data variables 32 + 32 + 8, and The bytes for the relayer variables, till we reach message field, and Start Searching From that position.

We can determine the exact position by testing and seeing the Log file.

[H-04] Attackers can Drain L2 Bridge because of not authorizing L1Sender or L2Receiver

Description

When Briding ETH/ERC20 from L1 to L2 the flow is Bridge -> Relayer -> Portal

In L2 we need to validate this execution before Bridging Tokens, where we need to validate that the tx sent first by L1 Bridge then to the Relayer then to Optimism Portal.

When Fetching deposit Transactions, we filter according to the Deposit event and the contract that emits the event only, So all Transactions that fired OptimismPortal::depositTransaction() whatever the caller was.

node/derive/src/attributes/mod.rs#prepare_payload_attributes

    async fn prepare_payload_attributes(
        &mut self,
        l2_parent: L2BlockInfo,
        epoch: BlockNumHash,
    ) -> PipelineResult<OpPayloadAttributes> {
        ...
>>      let deposit_filter = Filter::new()
            .address(self.rollup_cfg.deposit_contract_address)
            .event(SOON_DEPOSIT_EVENT_ABI)
            .select(header.number);
          let deposit_logs = self
              .receipts_fetcher
              .get_logs(&deposit_filter)
              .await
              .map_err(|e| PipelineError::Provider(e.to_string()).temp())?;

          let mut deposits = Vec::new();
          for (index, log) in deposit_logs.iter().enumerate() {
>>            let deposit = UserDeposited::try_from(log.clone())
                  .map_err(|err| PipelineError::ParseDepositTxErr(err.to_string()).temp())?;
              ...
          }
    }

Soon Nodes only checks for the Bridge selector to determine whether the tx was a bridged transaction or not using Bridge selector, finalizeBridgeETH, and finalizeBridgeERC20 selectors.

node/primitives/src/deposit.rs#UserDeposited

    fn try_from(log: Log) -> Result<Self, Self::Error> {
        ...
>>      let (ext, data) = ExternalData::extract(&opaque_data)?;
    }
// --------------
    pub fn extract(opaque_data: &[u8]) -> Result<(Self, Vec<u8>), DepositError> {
        ...
>>      let rest_start = Self::selector_end_index(opaque_data)?;
    }
// --------------
    fn selector_end_index(opaque_data: &[u8]) -> Result<usize, DepositError> {
        for i in 0..opaque_data.len() {
            if i + 4 <= opaque_data.len() {
                if opaque_data[i..i + 4] == ETH_DEPOSIT_SELECTOR
                    || opaque_data[i..i + 4] == ERC20_DEPOSIT_SELECTOR { ... }
            } else { ... }
        }
        ...
    }

In Soon, it retrieves OpaqueData and searches for ETH/ERC20_DEPOSIT_SELECTOR. In case it finds them, it starts retrieving Bridging parameters (from, to, amount, ...) from that selector.

But in Optimism, this data (OpaqueData), is sent by the sender, anyone on Optimism Can directly call optimismPortal, or call the relayer Directly, sending any message he needs.

This will make any user in L1 impersonate the Bridge, and Soon network will extract data and amount and transfer assets to the user thinking he already locked them in L1.

From our search we found that Soon Nodes don't even check for the to parameter, this is so weird, we think it May be implemented anywhere in the code, So even if this exists or not, the Attack is still applicable.

Proof of Concept

  • Attacker calls OptimismPortal::depositTransaction() directly
  • Attacker encoded finalizeBridgeETH() with parameters allowing him to take most of the Assets in vault_account (vault_account is the account paying ETH depositer in L2).
  • Soon Nodes extracted parameters and made the transaction ready to be executed
  • Soon Executer Module executed the transaction
  • The attacker took most of the ETH assets represented in Soon without paying nothing

Recommendations

To identify a Bridging Transaction either ETH/ERC20, we need to check for 3 things.

  1. The Sender should be L1CrossDomainMessanger
  2. The receiver should be L2Bridge
  3. The message send from L1CrossDomainMessanger encodes the msg.sender in _data field using encodeRelayL2Message(), which encodes with this signature relayMessage(uint256,address,bytes32,uint256,uint256,bytes). The second address should be the L1StandardBridge to only take the Relalated messages from the Bridge, not direct relayed messages.

[M-01] Users overpay fees when Bridging ETH/ERC20 tokens

Description

When Bridging tokens from L1 to L2 the execution is as follows. Bridge -> Relayer -> Optimism Portal

In The relayer when we send the message, we are increasing the gas by values representing

L1/CrossDomainMessanger.sol

    function sendMessage(bytes32 _target, bytes calldata _message, uint32 _minGasLimit) external payable {
        ...
        _sendMessage({
            _to: otherMessenger,
>>          _gasLimit: baseGas(_message, _minGasLimit),
            _value: msg.value,
            _data: encodeRelayL2Message(messageNonce(), msg.sender, _target, msg.value.ETHToRD(), _minGasLimit, _message)
        });

        ...
    }
// ---------------
    function baseGas(bytes calldata _message, uint32 _minGasLimit) public pure returns (uint64) {
        return
        // Constant overhead
        RELAY_CONSTANT_OVERHEAD
        // Calldata overhead
        + (uint64(_message.length) * MIN_GAS_CALLDATA_OVERHEAD)
        // Dynamic overhead (EIP-150)
        + ((_minGasLimit * MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR) / MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR)
        // Gas reserved for the worst-case cost of 3/5 of the `CALL` opcode's dynamic gas
        // factors. (Conservative)
        + RELAY_CALL_OVERHEAD
        // Relay reserved gas (to ensure execution of `relayMessage` completes after the
        // subcontext finishes executing) (Conservative)
        + RELAY_RESERVED_GAS
        // Gas reserved for the execution between the `hasMinGas` check and the `CALL`
        // opcode. (Conservative)
        + RELAY_GAS_CHECK_BUFFER;
    }

When Sending a Message from The Relayer, in Optimism The message is forwarded to the L2Relayer, so this implementation ensures the transaction will not revert when calling in the L2 relayer.

First, we use the same values, even EIP150, for depth calls, and these values depend on the values spent from the L2Relayer in Optimism. In Soon, we use Solana, which has a totally different gas calculation model that even depends on the computational cost, making all these values accurate.

In addition, soon, there will be no Relayer. The Bridged tokens are directly deposited to the L2 user.

  • Optimism: Layer2 Nodes -> L2 Relayer -> L2 Bridge
  • Soon: Layer2 Nodes -> L2 Bridge

src/libraries/Predeployes.sol

    /// @notice Address of the L2CrossDomainMessenger predeploy.
>>  bytes32 internal constant L2_CROSS_DOMAIN_MESSENGER =
        0x02c806312cb859f1bc25448e39f87aa09857d83ccb4a837df55648e000000000;

    ...
    /// @notice Address of the L2ERC721Bridge predeploy.
>>  bytes32 internal constant L2_STANDARD_BRIDGE = 0x02c806312cb859f1bc25448e39f87aa09857d83ccb4a837df55648e000000000;

    ...
    /// @notice Address of the L2ERC721Bridge redeploy.
>>  bytes32 internal constant L2_ERC721_BRIDGE = 0x02c806312cb859f1bc25448e39f87aa09857d83ccb4a837df55648e000000000;

The L2 ERC20 Bridge is the same as L2 ERC721 Bridge is the same as The Cross Domain Messanger, and the depositing to L2 take place via direct call through this address.

The Relayer gas value taken is constant, but in the Soon implementation, deposit ETH has its logic, and deposit ERC20 has its logic, so using the same values for both is incorrect.

In Optimism implementation, this gas is taken for the relayer, the gas for L2 Bridge execution is made by the user itself, as each token has its own gas for transferring like different tokens implementation in Solana in Token2022

The gas values we burn will:

  • make users overpay for gas
  • The Gas calculations are totally incorrect as it use EVM and Optimism gas calculations instead of Solana one (Solana have fixed 5000 lamport each transaction for example, and depends on computational cost)
  • In Correct Metering of resources L1 -> L2

Recommendations

Change Gas Calculations for Bridging or leave it to the users instead.

[M-02] Failure Parsed Deposit Transactions Logs, or sanitizing Skip all deposit transactions

Description

When Parsing deposit Transactions and pushing them to get prepared for execution, decoding them can revert in some cases, where some Deposit Transactions can't be Okay for decoding and processing.

This Error in Decoding can occuar, Soon Nodes, catch these Errors, to not revert the execution and return it in case it happen

node/derive/src/attributes/mod.rs#AttributesBuilder::prepare_payload_attributes()

    async fn prepare_payload_attributes(
        &mut self,
        l2_parent: L2BlockInfo,
        epoch: BlockNumHash,
    ) -> PipelineResult<OpPayloadAttributes> {
      for (index, log) in deposit_logs.iter().enumerate() {
          let deposit = UserDeposited::try_from(log.clone())
>>            .map_err(|err| PipelineError::ParseDepositTxErr(err.to_string()).temp())?;
          let sanitized_tx = deposit
              .to_sanitized_transaction(
                  header.clone(),
                  0,
                  index as u64 + 1, // 0 is for l1 block info
              )
>>            .map_err(|err| PipelineError::ParseDepositTxErr(err.to_string()).temp())?;
>>        deposits.push(sanitized_tx_to_bytes(sanitized_tx)?);
      }
    }

The ? in the last means we will return Error in case if occurs.

PipelineResult is a wrap of error, making the transaction able to return errors, and there are different types of error (this is not needed in our context, but to give the overall view).

node/derive/src/errors.rs

pub type PipelineResult<T> = Result<T, PipelineErrorKind>;
...
pub enum PipelineErrorKind {
    /// A temporary error.
    #[error("Temporary error: {0}")]
    Temporary(#[source] PipelineError),
    /// A critical error.
    #[error("Critical error: {0}")]
    `(#[source] PipelineError),
    /// A reset error.
    #[error("Pipeline reset: {0}")]
    Reset(#[from] ResetError),
}

Returning in case of failure in decoding in UserDeposited::try_from can occur. Many issues can make UserDeposited::try_from revert or even Paniced (We reported these issues before reporting this one). It is normal for User Deposit Log to fail to decode it.

We are aware that some errors like Critical ones should skip all deposits, especially if the error is not from the deposit Lop parsing itself, but in case of the error is in parsing the Log for a specific transaction, skipping the other ones is not ideal, as they will not get processed to.

The problem is that deposits are grouped as Batches, and all of them are processed, so in case only one of them fails, all other deposits will also get skipped.

This will make deposits in the Batch not processed, because of only one failed deposit.

This can be used in a Malicious Way to prevent processing other deposits, but the idea for that report is that dependency and the atomicity of parsing deposits, where if one deposit fails it should get skipped and not skip all other successful or deposits to be parsed next.

Recommendations

  • use match to skip the deposit in case of error and don't pupushst it into deposit array.
  • Having a varaible for failed deposits will be great, to be able to parse it again.

[M-03] All Bridged ERC721 tokens will get Lost

Description

In Optimism ERC721 tokens can be Bridged as well as ERC20/ETH.

Users can use L1ERC721Bridge for doing this process.

After Soon nodes graps Optimism Portal transactions, it checks for Bridged transaction. But they only check for ERC20/ETH

node/primitives/src/deposit.rs#UserDeposited

    fn try_from(log: Log) -> Result<Self, Self::Error> {
        ...
>>      let (ext, data) = ExternalData::extract(&opaque_data)?;
    }
// --------------
    pub fn extract(opaque_data: &[u8]) -> Result<(Self, Vec<u8>), DepositError> {
        ...
>>      let rest_start = Self::selector_end_index(opaque_data)?;
    }
// --------------
    fn selector_end_index(opaque_data: &[u8]) -> Result<usize, DepositError> {
        for i in 0..opaque_data.len() {
            if i + 4 <= opaque_data.len() {
>>              if opaque_data[i..i + 4] == ETH_DEPOSIT_SELECTOR
>>                  || opaque_data[i..i + 4] == ERC20_DEPOSIT_SELECTOR { ... }
            } else { ... }
        }
        ...
    }

We only Grap Transactions with ETH_DEPOSIT_SELECTOR or ERC20_DEPOSIT_SELECTOR, but we do not check for the ERC721 token selector.

This will make all Bridged ERC721 tokens gets Locked in L1ERC721Bridge and users will not be able to receive them in L2 nor gets them back in L1.

src/L1/L1ERC721Bridge.sol

    function _initiateBridgeERC721( ... ) ... {
        ...
        // Construct calldata for _l2Token.finalizeBridgeERC721(_to, _tokenId)
        bytes memory message = abi.encodeWithSelector(
>>          this.finalizeBridgeERC721.selector, _remoteToken, _localToken, _from, _to, _tokenId, _extraData
        );

        // Lock token into bridge
        deposits[_localToken][_remoteToken][_tokenId] = true;
>>      IERC721(_localToken).transferFrom({ from: _from, to: address(this), tokenId: _tokenId });

        // Send calldata into L2
>>      messenger.sendMessage({ _target: otherBridge, _message: message, _minGasLimit: _minGasLimit });
        emit ERC721BridgeInitiated(_localToken, _remoteToken, _from, _to, _tokenId, _extraData);
        ...
    }

All Bridged ERC721 tokens will be lost. In Optimism ERC721 tokens Bridged is supported, and We beleive Soon should work this way, since it includes L1ERC721Bridge.sol file, and put it in the Scope, making Briding ERC721 in Scope

Recommendations

Grap finalizeBridgeERC721(...) selector, and implement ERC721 Bridging mechanism

[M-04] There is no recovery mechanism for reverted Deposit Transactions

Description

When Bridging Tokens or relaying messages between two different blockchains, the Assets took by L1, can not reach L2. this can occur because of OOG, reverting from L2 receiver side, etc...

Bridge Protocol implement a recovery mechanism, in Optimism, if the relayed message gets reverted, it is stored, to then be able to manually execute by anyone.

src/universal/CrossDomainMessenger.sol#relayMessage()

    function relayMessage( ... ) ... {
        ....
        if (
>>          !SafeCall.hasMinGas(_minGasLimit, RELAY_RESERVED_GAS + RELAY_GAS_CHECK_BUFFER)
                || xDomainMsgSender != Constants.DEFAULT_L2_SENDER
        ) {
>>          failedMessages[versionedHash] = true;
            emit FailedRelayedMessage(versionedHash);
            ...
        }

        ...
    }

In CrossDomainMessenger, which is the version used in Optimism L2, in case of transaction failed, it is stored using its hash, in reverted transactions. to allow users to re-execute it again, as it can revert for several reasons.

But in Soon, this mechanism is not implemented, the deposit transactions don't have a recovery mechanism, if a transaction is reverted users will lose their money in L1, and there is no way to recover it by manually executing it, as the Relayer is the Bridger in Soon they are not separate like Optimism to make this functionality. Nor Soon nodes implement a way to recover these transactions.

Recommendations

Implement Recover mechanism for reverted L2 transaction either in L1 or in L2

[M-05] Users can generate yields at the expense of the Bridge by depositing small amounts

Description

When sending depositing ETH to the user in L2 there is a check for Rent, and in case the value is smaller than Rent, the value increases.

node/programs/bridge-program/src/processor.rs#Processor::deposit_eth

    fn deposit_eth( ... ) -> ProgramResult {
        ...
        // Transfer lamports from vault to depositor
        let min_balance = Rent::get()?.minimum_balance(0);
        let mut transfer_value = value as u64; // @audit-high This should be `mint` +

        // Only do this if the depositor account does not exist
>>      if transfer_value < min_balance && depositor_account.lamports() == 0 {
            transfer_value = min_balance;
        }
        ...
    }

If the value transfered is smaller than rent, we increase the value to reach min_balance, but User do not sent this value, Nodes are paying for the remaining themselves.

This will incentivize users to pay with the minimum amount of ETH in L1 accepted, and they may get excess rewards for that behavior.

Recommendations

Implement Loans for these types of transfers will be good.

[M-06] Renting check suppose All accounts with 0 length

Description

When depositing ETH in Layer2, we are making Rent check to pay the receiver in L2 the amount that will make him not exposed to insufficient funds to maintain rent exemption.

node/programs/bridge-program/src/processor.rs#Processor::deposit_eth

    fn deposit_eth( ... ) -> ProgramResult {
        ...
        // Transfer lamports from vault to depositor
>>      let min_balance = Rent::get()?.minimum_balance(0);
        let mut transfer_value = value as u64; // @audit-high This should be `mint` +

        // Only do this if the depositor account does not exist
        if transfer_value < min_balance && depositor_account.lamports() == 0 {
            transfer_value = min_balance;
        }

>>      let instruction = transfer(vault_account.key, depositor_account.key, transfer_value);
        ...
    }

When retrieving the minimum rent balance, we assume the depositor_account to be with no Data (EOA) account, but in case of Smart contract Wallet, or Program to be the receiver, the Rent is more, as the minimum_balance() should take the length of the program.

This will lead to incorrect calculations for Rent Saftly check making Smart Contract Wallets subjected to close the PDA. This can lead to a more issue by reinitializing the depositor contract if it works this way, etc...

Recommendations

Use depositor_account.data_len() instead of 0 when retrieving the minimum Rent Balance.

[M-07] from address should get DeAllaised if the Call is Bridging tokens

Description

Layer2 Nodes Listen for all L1 -> L2 deposit transactions, They filter Emits that only come from OptimismPortal and have TransactionDeposited event.

In 2: After retreiving Logs, this Logs are getting changed into UserDeposit Object to complete user Bridged L2 Transaction.

node/derive/src/attributes/mod.rs || node/node/src/derive/l1/event_provider.rs#parse_deposits_from_logs

    pub const SOON_DEPOSIT_EVENT_ABI: &str = "TransactionDeposited(address,bytes32,uint256,bytes)";
// --------------
    async fn prepare_payload_attributes( ... ) -> PipelineResult<OpPayloadAttributes> {
1:    let deposit_filter = Filter::new()
          .address(self.rollup_cfg.deposit_contract_address)
          .event(SOON_DEPOSIT_EVENT_ABI)
          .select(header.number);
      let deposit_logs = self
          .receipts_fetcher
          .get_logs(&deposit_filter)
          .await
          .map_err(|e| PipelineError::Provider(e.to_string()).temp())?;

      let mut deposits = Vec::new();
      for (index, log) in deposit_logs.iter().enumerate() {
2:        let deposit = UserDeposited::try_from(log.clone())
              .map_err(|err| PipelineError::ParseDepositTxErr(err.to_string()).temp())?;
          ...
      }
    }

Calling UserDeposited::try_from() will reterive the from address as EVM 20 bytes address.

node/primitives/src/deposit.rs -> impl TryFrom for UserDeposited

    fn try_from(log: Log) -> Result<Self, Self::Error> {
        let opaque_data = log.data().data.to_vec();

>>      let from = Address::from_word(log.topics()[1]);
        ...
    }

The problem is that we retrive the address as it is, but in Layer one if the caller of the deposit is not an EOA, the address is getting alaised.

src/L1/OptimismPortal.sol

    address from = msg.sender;
    if (msg.sender != tx.origin) {
>>      from = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
    }

The sender will not be the transaction origin in case of Bridging Tokens ERC20/ERC721 or Relaying messages, as the caller for these two executions are contracts L1 Relayer, L1 Bridge, ... which will make L2 nodes retrieve wrong from address making authorization process for the original caller fail, in case some executions are restricted to a given from address, like authenticating Bridging tokens and Relaying messages.

Recommendations

Don't Alaise addresses in L1, alaising is made for EVM L2 to distinguish between addresses in the different blockchains, but since SVM has its own address structure, aliasing from L1 is not needed.

[M-08] uint64 is too small For 18 decimal Tokens deposited in Soon

Description

When depositing Tokens Bridged from L1 to L2, the amount it being taken as u64 value.

    fn deposit_erc20( ... ) -> ProgramResult {
        ...

        // Mint spl tokens to the target
        let spl_token_decimals = Mint::unpack(&spl_token_mint_account.data.borrow())?.decimals;
>>      let amount = Self::convert_decimal(amount, SPL_SHARE_DECIMAL, spl_token_decimals);
        msg!("Deposit amount: {}", amount);

        let instruction = mint_to(
            spl_token_program_account.key,
            spl_token_mint_account.key,
            depositor_associated_token_account.key,
            spl_token_owner_account.key,
            &[],
>>          amount as u64,
        )?;
        ...

        Ok(())
    }
// -----------------
    pub fn convert_decimal(amount: u128, from_decimal: u8, to_decimal: u8) -> u128 {
        if from_decimal > to_decimal {
            let conversion_rate: u128 = 10_u128.pow((from_decimal - to_decimal) as u32);
            amount / conversion_rate
        } else {
            let conversion_rate: u128 = 10_u128.pow((to_decimal - from_decimal) as u32);
>>          amount * conversion_rate
        }
    }

the maximum number for u64 is 18446744073709551615 ~= 18.45e18. And the SPL token is created with decimals. This will make transferring more than 19 token of decimal 18 will revert because of downcasting. where solana thread revert automaticaly in case of Downcasting.

This will prevent users from depositing tokens with 18 decimals in Soon, and there transactions will revert and they will lose The money they deposited in L1.

Recommendations

Created SPL tokens, should handle amount with decimals, The standard in Solana is 9 decimals, but accepts decimals to be put, since mint_to is with amount of type u64 changing it may be incorrect.

In L1 Bridge, we should ensure that the amount to be deposited in Soon in Remote Token (The final amount to be minted) will not exceed type(64).max in L1 Bridge side.

[M-09] DepositERC20 Is not enforcing success

Description

When Soon Nodes The Executor checks for the transactions that must succeed, as no one of these transactions should revert when executing a given Batch of transactions.

node/executor/src/worker.rs#WorkerImpl::run_once()

    pub fn run_once(&mut self) {
        while let Ok(task) = self.task_receiver.recv() {
            ...

            let output = processor.load_and_execute_sanitized_transactions(
                bank.as_ref(),
                transactions,
                results,
                &environment,
                &config,
            );
            // Validate native transactions must succeed
            for (result, tx) in output.execution_results.iter().zip(transactions.iter()) {
>>              if tx.has_must_succeed_native_instruction() {
                    if let Err(err) = result.flattened_result() {
                        let _ = result_sender
                            .send((task.clone(), Err(DerivedTxsExecutedFailed(err.to_owned()))));
                        return;
                    }
                }
            }
            ...
        }
    }

The transactions that must succeed, are Bridge transactions, and The Updating L1.

node/primitives/src/native_tx/instruction.rs#HasNativeInstruction::has_must_succeed_native_instruction()

    fn has_must_succeed_native_instruction(&self) -> bool {
        self.has_update_l1_block_info_instruction() || self.has_must_succeed_bridge_instruction()
    }

But the enforcing for Bridge Transaction is only enforcing DepositETH nor DepositERC20

    fn has_must_succeed_bridge_instruction(&self) -> bool {
        self.message()
            .program_instructions_iter()
            .any(|(program, ix)| {
                program == &bridge::id()
                    && BridgeInstruction::unpack(&ix.data)
>>                      .map(|ix| ix.must_succeed())
                        .unwrap_or(false)
            })
    }
// ---------------
    pub fn must_succeed(&self) -> bool {
>>      matches!(self, BridgeInstruction::DepositETH { .. })
    }

Only DepositETH enforces the must_success, but this check is not enforced for ERC20 deposits.

All SPL tokens are created from The Bridge contract itself, the SPL tokens created will not be a random tokens that some of them can be malicious or anything, all of them are created by admins using create_spl() from the L2 Bridgge, making them a trusted tokens, The revert can occur because of insufficient gas, etc...

For example, the transaction can be the last one, and goes with OOG error, or the depositing ERC20 can revert (it can revert from downcasting, and we mentioned this in a separate report, etc...) but since these transactions are fired from L1 they should get executed in L2.

Recommendation

Add DepositERC20 into must_succeed instructions from the Bridge.

    pub fn must_succeed(&self) -> bool {
-       matches!(self, BridgeInstruction::DepositETH { .. })
+       matches!(self, BridgeInstruction::DepositETH { .. } | BridgeInstruction::DepositERC20 { .. })
    }

[M-10] Soon is not handling Direct deposits from Portal

Description

In Optimism Blockchains, users can deposit ETH directly into layer 2 using the Optimism Portal; this can be done by calling OptimsimPortal directly.

In this process, the _data field can be empty. Anyone can actually call OptimimPortal directly without providing the Data field, and one of these usages is the direct transfer of ETH, as we said.

src/L1/OptimismPortal#_depositTransaction

    function _depositTransaction(
        bytes32 _to,
        uint256 _mint,
        uint256 _value,
        uint64 _gasLimit,
        bool _isCreation,
>>      bytes memory _data
    ) ... {
      ...
>>    bytes memory opaqueData = abi.encodePacked(_mint.ETHToRD(), _value.ETHToRD(), _gasLimit, _isCreation, _data);
      ...
    }

When encoding opaqueData, we use encodePacked 5 variables, [uint256, uint256, uint64, bool, _data], the total length will be 32 + 32 + 8 + 1 + 0 (empty data) = 73.

This will introduce issues in processing deposits from Soon Nodes side, as they deal with opaqueData as daat with +73 length.

node/primitives/src/deposit.rs#ExternalData

impl ExternalData {
    pub fn extract(opaque_data: &[u8]) -> Result<(Self, Vec<u8>), DepositError> {
        ...

        let rest_start = Self::selector_end_index(opaque_data)?;
>>      let extra_data = opaque_data[73..rest_start].to_vec();
        let rest = opaque_data[rest_start..].to_vec();

        ...
    }
}

We retrieve the extra_data by accessing element 73, but in case of empty data the opaque_data length is 73 accessing element 73 means out of-bound array access, leading to the panic of the thread execution in case of reaching this line.

This will make all Direct ETH deposits from L1 to L2 through OptimismPortal get los perminantly.

Recommendations

Check for the length of the opaque_data before extracting extra_data and in case it is 73 length this means there is no extra_data to extract

[L-01] Creating Contracts L1 -> L2 is Not Supported in Soon

Description

In L1 Soon Accepts passing _isCreation, making the transaction contract creation. This action is supported in L1.

src/L1/OptimismPortal.sol#_depositTransaction

    function _depositTransaction(
        bytes32 _to,
        uint256 _mint,
        uint256 _value,
        uint64 _gasLimit,
>>      bool _isCreation,
        bytes memory _data
    ) ... {
        ...

        // Compute the opaque data that will be emitted as part of the TransactionDeposited event.
        // We use opaque data so that we can update the TransactionDeposited event in the future
        // without breaking the current interface.
>>      bytes memory opaqueData = abi.encodePacked(_mint.ETHToRD(), _value.ETHToRD(), _gasLimit, _isCreation, _data);

        // Emit a TransactionDeposited event so that the rollup node can derive a deposit
        // transaction for this deposit.
        emit TransactionDeposited(from, _to, DEPOSIT_VERSION, opaqueData);
    }

But in Layer2, this action is not supported. The parameter is passed until it goes to L2Bridge and then passed as it is without any action.

node/programs/bridge-program/src/processor.rs#Processor::deposit_eth

    fn deposit_eth(
        ...
>>      _is_creation: bool,
        ...
    ) -> ProgramResult { ... }

No Action is Taken after that, no deploying occurs if _is_creation is true.

This will make Creation Transactions from L1 not doing the right action in L2

Recommendations

Since Creating a Contract from EVM data to SVM can be too difficult. SVM contract (program) deploying is not even a bytecode-based deployment like EVM, We see disabling the variable to be always false is true, and revert if it is set to True in Layer1 OptimismPortal

[L-02] Soon Nodes do not decode extraData after extracting it.

Description

When Bridging ETH/ERC20 from L1 to L2 users can provide extraData parameter, this parameter can be used in the validation process, where it is emited in Layer2 as well to allow users to take certin actions etc...

src/universal/StandardBridge.sol

    function _initiateBridgeETH(
        address _from,
        bytes32 _to,
        uint256 _amount,
        uint32 _minGasLimit,
>>      bytes memory _extraData
    ) ... {
        ...
        messenger.sendMessage{ value: _amount }({
            _target: otherBridge,
>>          _message: abi.encodeWithSelector(this.finalizeBridgeETH.selector, _from, _to, _amount.ETHToRD(), _extraData),
            _minGasLimit: _minGasLimit
        });
    }

This parameter is encoded using Solidity encodeWithSelector encoding bytes is like arrays where

  • 1st slot: includes offset
  • 2nd slot: includes the length of the bytes array
  • 3.. slot: include Bytes data

Bytes are padded into 32, so if length of bytes is 20 they will accumulate the next slot totally by padding the rest of 0, etc with the normal encoding in Solidity we know.

In Soon, this data is extracted directly, by reading the Bytes for it.

node/primitives/src/deposit.rs#EthDepositData & #Erc20DepositData

impl TryFrom<Vec<u8>> for EthDepositData {
    ...
    fn try_from(value: Vec<u8>) -> Result<Self, Self::Error> {
        let from = Address::from_slice(&value[12..32]);
        let to = Pubkey::try_from(&value[32..64]).map_err(|_| DepositError::InvalidPubKey)?;
        let amount = U256::from_be_slice(&value[64..96]);
        // TODO: current extra data needs to be fixed size of 32 bytes, need to update this later
>>      let mut extra_data = value[96..].to_vec();
>>      extra_data.resize(32, 0);
        ...
    }
}
// ----------------------
impl TryFrom<Vec<u8>> for Erc20DepositData {
    ...
    fn try_from(value: Vec<u8>) -> Result<Self, Self::Error> {
        let l2_contract =
            Pubkey::try_from(&value[0..32]).map_err(|_| DepositError::InvalidPubKey)?;
        let l1_contract = Address::from_slice(&value[44..64]);
        let from = Address::from_slice(&value[76..96]);
        let to = Pubkey::try_from(&value[96..128]).map_err(|_| DepositError::InvalidPubKey)?;
        let amount = U256::from_be_slice(&value[128..160]);
>>      let extra_data = value[160..].to_vec();

        ...
    }
}

In extracting data in the case of Bridging ETH we resize it to 32 bytes, and in ERC20 we leave it as it is.

We can't determine the purpose of resizing to 32 bytes in Ethereum, but the overall process will take all extraData without decoding it, which means the extraData will contain.

  • First 32 bytes will have the offset
  • Second 32 bytes will have the length
  • Third 32 bytes will have the data itself and can increase to include more than one slot

In the case of ERC20, we are not decoding data, this means the data we pass will contains offset, length, and can have paddings in case of the data length is smaller than N*32 of zero, making the value incorrect.

And in the case of ETH deposit, it is worth it. As we only take the first 32 bytes, which will be just the offset value, making the extra data lost, as this value will mostly be the same for all dataValues.

This will make extraData value gets retrieved incorrectly, corrupting and disabling the ability of using it.

Recommendations

Skip the first 32 bytes, then extract the next 32 bytes to get the Length, then slice the array to that value to extract the exact data passed by the user from Layer1

[L-03] The depositing value in Soon is uint128 and uint256 in Ethereum

Description

The ERC20 Bridge For Ethereum Accepts any value to be deposited with any token even if it exceeds type(uint128).max, there are no restrictions for the amount or token to Bridge.

But in Soon, The value of ERC20 token is getting downcasting to uint128, where deposit_erc20 taken the amonut as u128

node/programs/bridge-program/src/instruction.rs

pub fn deposit_erc20(
    local_token: Pubkey,
    remote_token: Address,
    from: Address,
    to: Pubkey,
>>  amount: u128,
) -> Instruction { ... }

The value is getting retrieved as uint256, but then gets downcasting to u128, so in case the amount if ERC20 token to bridge exceeds U128::MAX the downcasting will revert and return an error.

node/primitives/src/deposit.rs#UserDeposited

impl UserDeposited {
    pub fn to_sanitized_transaction( ... ) -> Result<SanitizedTransaction, DepositError> {
        let instructions = match self.deposit_type {
            ...
            DepositType::ERC20(data) => vec![deposit_erc20(
                data.l2_contract,
                data.l1_contract.into_array().into(),
                data.from.into_array().into(),
                data.to,
>>              data.amount
                    .try_into()
                    .map_err(|_| DepositError::ParseDepositTxError("Invalid amount".to_string()))?,
            )],
        };
        ...
    }
}

This will make users lose all the tokens he deposited, as the his deposit will not get fired in Soon bridge, and will fail.

This downcasting is also using in depositing ETH, but U128::MAX is enough for Ethereum, but as Optimism Bridge supports all ERC20 tokens, we can't determine the amount of tokens to be Bridged.

Recommendations

In L1 Bridge, check that the amount is less than or equal type(uint128).max

[I-01] Soon Bridge is not Processing Relayer messages

Description

In Optimism Users can Bridge ETH/ETC20 tokens, as well as sending plan Messages. Where users can directly use L1CrossDomainMessanger to realy messages without transferring tokens

src/universal/CrossDomainMessenger.sol#sendMessage

    function sendMessage(bytes32 _target, bytes calldata _message, uint32 _minGasLimit) external payable {
        if (isCustomGasToken()) {
            require(msg.value == 0, "CrossDomainMessenger: cannot send value with custom gas token");
        }

        // Triggers a message to the other messenger. Note that the amount of gas provided to the
        // message is the amount of gas requested by the user PLUS the base gas value. We want to
        // guarantee the property that the call to the target contract will always have at least
        // the minimum gas limit specified by the user.
        _sendMessage({
            _to: otherMessenger,
            _gasLimit: baseGas(_message, _minGasLimit),
            _value: msg.value,
            _data: encodeRelayL2Message(messageNonce(), msg.sender, _target, msg.value.ETHToRD(), _minGasLimit, _message)
        });

In Soon, we are not handling these messages, nor there is a mechanism to process them in Soon Network. Soon Network Just listen for ERC20/ETH deposits.

node/primitives/src/deposit.rs#UserDeposited

    fn try_from(log: Log) -> Result<Self, Self::Error> {
        ...
>>      let (ext, data) = ExternalData::extract(&opaque_data)?;
    }
// --------------
    pub fn extract(opaque_data: &[u8]) -> Result<(Self, Vec<u8>), DepositError> {
        ...
>>      let rest_start = Self::selector_end_index(opaque_data)?;
    }
// --------------
    fn selector_end_index(opaque_data: &[u8]) -> Result<usize, DepositError> {
        for i in 0..opaque_data.len() {
            if i + 4 <= opaque_data.len() {
>>              if opaque_data[i..i + 4] == ETH_DEPOSIT_SELECTOR
>>                  || opaque_data[i..i + 4] == ERC20_DEPOSIT_SELECTOR { ... }
            } else { ... }
        }
        ...
    }

This will make all relayered Messages get Lost, and not processed in Soon Blockchain, when relayering them from L1.

Recommendations

Implement Relaying messages only mechanism in Soon

[I-02] Gas value is set by users in L1 and not used in L2

Description

When Bridging ETH/ERC20 tokens from L1 to L2, users set the gas Limit themselves, this gas limit is to be used in L2 to call the transaction with it, this gas is then going to CrossDomainMessanger to add relayer fees, then through Portal, The gas is Burned through L1 to determine the resources and metering calculations.

    function _initiateBridgeETH(
        address _from,
        bytes32 _to,
        uint256 _amount,
>>      uint32 _minGasLimit,
        bytes memory _extraData
    )
        internal
    {
        ...

        messenger.sendMessage{ value: _amount }({
            _target: otherBridge,
>>          _message: abi.encodeWithSelector(this.finalizeBridgeETH.selector, _from, _to, _amount.ETHToRD(), _extraData),
            _minGasLimit: _minGasLimit
        });
    }

In Soon Nodes, after parsing Transactions, and retrives Deposits, then execute them, in batches, the gas provided for batches is the overall gas for all transactions.

node/derive/src/attributes/mod.rs#AttributesBuilder::prepare_payload_attributes

  async fn prepare_payload_attributes(
      &mut self,
      l2_parent: L2BlockInfo,
      epoch: BlockNumHash,
  ) -> PipelineResult<OpPayloadAttributes> {
      ...
      Ok(OpPayloadAttributes {
          payload_attributes: PayloadAttributes {
              timestamp: next_l2_time,
              prev_randao: B256::default(),
              suggested_fee_recipient: SEQUENCER_FEE_VAULT_ADDRESS,
              parent_beacon_block_root: parent_beacon_root,
              withdrawals,
          },
          transactions: Some(txs),
          no_tx_pool: Some(true),
>>        gas_limit: Some(u64::from_be_bytes(
              alloy_primitives::U64::from(sys_config.gas_limit).to_be_bytes(),
          )),
          eip_1559_params: eip_1559_params_from_system_config(
              &self.rollup_cfg,
              l2_parent.block_info.timestamp,
              0,
              &sys_config,
          ),
      })
  }

When Executing The patch of transactions, there is no check, calculation, or anything else for the gas Provided by the users.

node/executor/src/worker.rs#WorkerImpl::run_once()

    pub fn run_once(&mut self) {
        while let Ok(task) = self.task_receiver.recv() {
            ...

            // NOTE: when this batch is dropped, the lock is released.
            // In solana, the releasing should be done after bank_commit to avoid cross-thread conflicts.
            // However, here we can just release after execution and before commit.
            // Because the lock here is just for avoiding conflict in-thread.
            let batch = verifier.lock_batch(transactions);
            let results = verifier.validate_batch(&batch, true);
>>          let output = processor.load_and_execute_sanitized_transactions(
                bank.as_ref(),
                transactions,
                results,
                &environment,
                &config,
            );
            // Validate native transactions must succeed
>>          for (result, tx) in output.execution_results.iter().zip(transactions.iter()) {
                if tx.has_must_succeed_native_instruction() {
                    if let Err(err) = result.flattened_result() {
                        let _ = result_sender
                            .send((task.clone(), Err(DerivedTxsExecutedFailed(err.to_owned()))));
                        return;
                    }
                }
            }
            self.report_tx_error_metrics(&output.error_metrics);
            self.report_execute_details_timings(&output.execute_timings.details);
            self.report_execute_accessories(&output.execute_timings.execute_accessories);
            let _ = result_sender.send((task.clone(), Ok(output)));
        }
    }

The Worker Calls all the Batch Transactions at once using its system Configuration gas limit, and then outputs the result if all of them success of not.

There is no check for the gas used per transaction and comparing it aganist the gas limit provided by each user.

This will Allow users to set the gas limit to a minimum value forced by Portal, guaranteeing the success of their transaction whenever it occurs.

Transactions differ, where there is ETH deposit, and ERC20 deposit. And ERC20 is created in case it does not exist. Allow users to provide less value of gas than required to execute there transaction.

Recommendations

Implement Gas mechanism in Soon, This can be hard as SVM gas model is different from EVM, having a minimum value for Bridging ETH, and ERC20 for minGasLimit is a good partial mitigation.