Upload report

sherlock-audit · Nov 6, 2024 · effa51c · effa51c
1 parent 2b1824a
commit effa51c
Showing 1 changed file with 364 additions and 0 deletions.
diff --git a/README.md b/README.md
@@ -0,0 +1,364 @@
+# Issue H-1: [H-1] - User can deposit unauthorized tokens, leading to incorrect crediting of USDC on the other chain. 
+
+Source: https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract-judging/issues/40 
+
+## Found by 
+0rpse, 0xNirix, LZ\_security, Q7, S3v3ru5, Tendency, calc1f4r, chinepun, dod4ufn, g, krikolkk, pashap9990, sh1v, shaflow01, ubermensch, xKeywordx
+### Summary
+
+There are no checks to ensure that the `deposit_token` matches the `allowed_token` in the `solana_vault::deposit` function. This allows an attacker to deposit any tokens and get minted USDC on the other chain. The system assumes that an allowed token is deposited every time and this assumption is wrong.
+
+### Root Cause
+
+In [deposit.rs](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/vault_instr/deposit.rs#L22), the `Deposit` struct lacks a constraint to verify that `deposit_token.key()` matches the `allowed_token.mint_account`.
+
+This check could also be added inside the `solana_vault::deposit` or the `deposit.rs::apply` functions directly, but currently, none of these functions have this check either.
+
+`deposit_params.token_hash` is a user-controlled input param provided by the user when they call the `solana_vault::deposit` function. It is intended to represent the hash of the token the user is depositing. The problem is that there is no enforced relationship with `deposit_token`. There is no constraint that enforces that `deposit_params.token_hash` corresponds to the actual `deposit_token` mint provided in the accounts.
+
+### Internal pre-conditions
+
+1. The `allowed_token` account is configured to accept a specific token (USDC) with `allowed_token.allowed == true`.
+2. The `allowed_token.mint_account` is set to the mint address of the allowed token (USDC).
+3. The vault is initialized and operational, expecting deposits of the allowed token.
+
+### External pre-conditions
+
+1. The attacker possesses another token, e.g., WIF or another memecoin.
+2. The attacker has a token account associated with this memecoin and owns, let's say 10.000 tokens which are worth 1$. Assume a price/coin of 0.0001$.
+3. The attacker knows the `token_hash` corresponding to the allowed token (USDC).
+
+### Attack Path
+
+The attacker calls the deposit Function and provides:
+- `WIF` as `deposit_token`
+-  with an amount of `10.000` 
+-  `deposit_params.token_hash` that corresponds to USDC token_hash
+- includes the `allowed_token` that the Vault is configured to accept (USDC).
+
+Program Execution:
+
+- The function `deposit.rs::transfer_token_ctx` transfers the tokens from the attacker's `user_token_account` to the vault's `vault_token_account`.
+- Due to the missing constraint, the program does not verify that `deposit_token.key()` matches `allowed_token.mint_account`.
+- The `deposit.rs::apply` function increments `vault_authority.deposit_nonce` by 1.
+- After that, the `apply` function creates the `VaultDepositParams` object, setting `token_hash` to be equal to whatever `token_hash` the user provided inside the `DepositParams` struct.
+- The program records the deposit as if the allowed token (USDC) was deposited.
+- It sends a message to the other chain indicating that the allowed token was deposited.
+
+
+Resulting Impact:
+
+- On the other chain, the attacker is credited with USDC equivalent to the amount of tokens deposited (10.000 USDC).
+- The vault holds the WIF tokens, which are worth 1$ (for the sake of this example). The attacker can even create a random coin with no value and make the deposit.
+
+
+### Impact
+
+For the Protocol:
+
+- The protocol suffers a financial loss equivalent to the amount of USDC incorrectly credited to the attacker on the other chain.
+- The vault ends up holding unauthorized tokens instead of the intended allowed tokens.
+
+
+For the Attacker:
+
+- The attacker gains USDC on the other chain without depositing the equivalent value of the allowed token.
+- The attacker can drain the vault.
+
+
+### PoC
+
+Not necessary. Check the `solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/vault_instr/deposit.rs::Deposit` struct definition [here](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/vault_instr/deposit.rs#L22). There are no checks that enforce `deposit_token == allowed_token`. 
+
+You can also check the Solana Vault deposit function implementation `solana-vault/packages/solana/contracts/programs/solana-vault/src/lib.rs::deposit` [here](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/lib.rs#L22) and see that it doesn't have this check, nor does the `solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/vault_instr/deposit.rs::apply` function -- check it [here](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/vault_instr/deposit.rs#L107).
+
+### Mitigation
+
+Add a new `constraint` in `deposit.rs::Deposit` struct.
+
+Update the `deposit_token` account definition in `deposit.rs` to include a `constraint` that ensures the `deposit_token` mint matches the `allowed_token.mint_account`.
+
+
+```rust
+#[account(
+    constraint = deposit_token.key() == allowed_token.mint_account @ VaultError::InvalidDepositToken
+)]
+```
+
+
+
+## Discussion
+
+**sherlock-admin2**
+
+The protocol team fixed this issue in the following PRs/commits:
+https://github.com/OrderlyNetwork/solana-vault/pull/4
+
+
+# Issue H-2: A malicious user can withdrawals another user's money 
+
+Source: https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract-judging/issues/99 
+
+## Found by 
+0rpse, 0xBoboShanti, Q7, S3v3ru5, Silvermist, Tendency, chinepun, dod4ufn, g, infect3d, krikolkk, shaflow01, ubermensch
+### Summary
+
+A shared vault authority signing mechanism will cause unauthorized withdrawals for users, as User A can withdraw funds belonging to User B.
+
+### Root Cause
+In the [OAppLzReceive](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/a40ed80ce4a196bc81bfa6dfb749c19b92c623b0/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/oapp_instr/oapp_lz_receive.rs#L113-L114), the `vault_authority_seeds` are shared across all users, allowing any user with valid withdrawal parameters to use the same PDA signing authority. As a result, any valid withdrawal request can be signed by the vault without distinguishing which user is performing the withdrawal. 
+
+Also, there is no check that the wallet receiving the funds belongs to the same user for whom the withdrawal request was initiated. The system only checks that the withdrawal message comes from a valid sender (peer.address == params.sender), but does not verify that the user account corresponds to the sender.
+
+### Internal pre-conditions
+
+_No response_
+
+### External pre-conditions
+
+_No response_
+
+### Attack Path
+
+1. User B initiates a withdrawal on the Ethereum side and a valid withdraw message is sent to the Solana. 
+2. User A accesses the valid withdrawal messages corresponding to User B's account and calls the function before User B.
+3. Since there is no check to ensure the User A uses a withdrawal message corresponding to his account, the withdraw is successfully executed and User A steals User B's money.
+
+### Impact
+
+The attacker steals the entire withdrawn amount from User B's account without any corresponding loss.
+
+### PoC
+
+_No response_
+
+### Mitigation
+
+_No response_
+
+# Issue M-1: Ordered delivery can not be disabled 
+
+Source: https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract-judging/issues/8 
+
+## Found by 
+g
+### Summary
+
+[`nextNonce()`](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/sol-cc/contracts/SolConnector.sol#L100-L102) always returns a non-zero value. Because of this, ordered delivery can not be disabled. `nextNonce()` [should return zero](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/sol-cc/contracts/layerzerolabs/lz-evm-oapp-v2/contracts/oapp/OAppReceiverUpgradeable.sol#L79-L80) to signal to the executor that ordered delivery is disabled. 
+
+### Root Cause
+
+`SolConnector`'s [`nextNonce()`](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/sol-cc/contracts/SolConnector.sol#L100-L102) should return 0 when `orderDelivery` is false.
+
+### Internal pre-conditions
+
+1. Admin sets `orderDelivery` to false with [`setOrderDelivery()`](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/sol-cc/contracts/SolConnector.sol#L121-L125)
+
+### External pre-conditions
+
+None
+
+### Attack Path
+
+1. Executor (this is an off-chain program) is about to execute a message with a nonce of 1 but first checks `nextNonce()` and gets a non-zero value. 
+2. Executor will not execute any messages with a nonce that do not match the `nextNonce()`.
+
+### Impact
+
+Disabling ordered delivery/execution will not work.
+
+### PoC
+
+_No response_
+
+### Mitigation
+
+Consider modifying `nextNonce()` to return 0 when `orderDelivery` is false.
+
+```solidity
+function nextNonce(uint32 /*_srcEid*/, bytes32 /*_sender*/) public view override returns (uint64 nonce) {
+    if (orderDelivery) {
+        nonce = inboundNonce + 1;
+    }
+}
+```
+
+
+
+## Discussion
+
+**sherlock-admin2**
+
+The protocol team fixed this issue in the following PRs/commits:
+https://github.com/OrderlyNetwork/sol-cc/pull/1/commits/6afb68b75b604cd133d5838484bb1bede23a79b1
+
+
+# Issue M-2: Executor will attempt unordered execution of messages because ordered execution option is not set 
+
+Source: https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract-judging/issues/12 
+
+## Found by 
+g
+### Summary
+
+The **ordered execution option** is [not set](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/sol-cc/contracts/SolConnector.sol#L92-L95) when doing `_lzSend()` in `SolConnector.withdraw()`. If it is not set, the Executor will attempt unordered execution of messages even when `SolanaVault` expects them to be [ordered](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/oapp_instr/oapp_lz_receive.rs#L94-L99).
+
+### Root Cause
+
+In [`SolConnector.withdraw():92-95`](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/sol-cc/contracts/SolConnector.sol#L92-L95), the ordered execution option is not set. Not setting the ordered execution option will mean the Executor will [default](https://docs.layerzero.network/v2/developers/evm/oapp/message-design-patterns#code-example-2) to unordered execution. 
+
+> If you do not pass an ExecutorOrderedExecutionOption in your _lzSend call, the Executor will attempt to execute the message despite your application-level nonce enforcement, leading to a message revert.
+
+### Internal pre-conditions
+
+None
+
+### External pre-conditions
+
+None
+
+### Attack Path
+
+1. An Operator or Manager executes the withdraw action on the Ledger which calls `SolConnector`'s [`withdraw()`](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/sol-cc/contracts/SolConnector.sol#L77-L98).
+2. In Solana, the Executor (an off-chain program) processes the executor options for the LayerZero message and sees that it is not set as ordered execution. The Executor attempts to execute the LayerZero message out-of-order after its verification.
+3. The message reverts and it will not be executed until it is manually retried.
+
+### Impact
+
+`SolanaVault` can be configured for [ordered delivery](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/oapp_instr/oapp_lz_receive.rs#L94-L99), but the Executor will always attempt unordered execution. This breaks core contract functionality, which is ordered delivery/execution.  
+
+### PoC
+
+_No response_
+
+### Mitigation
+
+Consider adding the ordered execution option when calling `_lzSend()` in `SolConnector.withdraw()`.
+
+```solidity
+    bytes memory withdrawOptions = OptionsBuilder.newOptions().addExecutorLzReceiveOption(
+        msgOptions[uint8(MsgCodec.MsgType.Withdraw)].gas,
+        msgOptions[uint8(MsgCodec.MsgType.Withdraw)].value
+    ).addExecutorOrderedExecutionOption();
+```
+
+Since ordered execution can be toggled in `SolanaVault`, this ordered execution option should also be made configurable.
+
+# Issue M-3: Excess fees refunded by endpoint will be stuck in `solana-vault` as it has no instructions to transfer them out 
+
+Source: https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract-judging/issues/121 
+
+## Found by 
+0xDemon, Anirruth, DenTonylifer, LZ\_security, Silvermist, TopStar, ZanyBonzy, bareli, infect3d, turvec
+### Summary
+
+Refunded fees will be stuck in `solana-vault` program when the excess paid fees will be refunded.
+
+`solana-vault::oapp_quote` allow to get [an **estimate of the required fees**](https://docs.layerzero.network/v2/developers/solana/oft/native#estimating-fees-and-calling-send) to sucessfully transmit a message from the src chain to the dst chain. If there is an excess of `fees` once finally executed, [the excess is refunded](https://docs.layerzero.network/v2/developers/evm/oapp/overview#example-sending-a-string).
+
+### Root Cause
+
+- [`solana-vault::oapp_quote`](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/oapp_instr/oapp_quote.rs#L37-L37) gives an estimate of the required fees
+- there is no way to withdraw the excess fees in solana-vault
+
+### Internal pre-conditions
+
+- the quoted fees are sent
+
+
+### External pre-conditions
+
+- Fees ar lower than expected and excess is refunded
+
+
+### Attack Path
+
+- Caller of `solana-vault::deposit` uses the LayerZero `quote` function to estimate the fees
+- The fees are over-estimated and the excess is refunded
+- As there is no function to recover the fees, fees are stuck
+
+
+### Impact
+
+- Fees stuck in contract
+
+
+### PoC
+
+_No response_
+
+### Mitigation
+
+Add a mechanism to be able to withdraw the fees
+See an implementation of fee withdrawing in another project: https://github.com/LayerZero-Labs/LayerZero-v2/blob/main/packages/layerzero-v2/solana/programs/programs/simple-messagelib/src/instructions/admin/withdraw_fees.rs
+
+# Issue M-4: Fees is not tabulated properly in oapp_lz_receive.rs 
+
+Source: https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract-judging/issues/124 
+
+## Found by 
+LZ\_security, ZanyBonzy, pashap9990, peanuts, shaflow01
+### Summary
+
+In [`oapp_lz_receive.rs`](https://github.com/sherlock-audit/2024-09-orderly-network-solana-contract/blob/main/solana-vault/packages/solana/contracts/programs/solana-vault/src/instructions/oapp_instr/oapp_lz_receive.rs#L117), the fees is calculated through the withdraw params, and is deducted from the actual token amount to be transferred to the user. `amount_to_transfer = withdraw_params.token_amount - withdraw_params.fee;`
+
+This leaves the fee amount in the `vault_deposit_wallet`. With many messages that are being received, `vault_deposit_wallet` will not know how much fees are collected.
+
+### Root Cause
+
+In oapp_lz_receive, the actual amount to transfer to the user is the amount minus fees.
+
+```rust
+         // msg!("Withdraw amount = {}", withdraw_params.token_amount);
+ >          let amount_to_transfer = withdraw_params.token_amount - withdraw_params.fee;
+            transfer(
+                ctx.accounts
+                    .transfer_token_ctx()
+                    .with_signer(&[&vault_authority_seeds[..]]),
+                amount_to_transfer,
+            )?;
+
+```
+
+`transfer_token_ctx` transfers funds from the `vault_deposit_wallet` to the `user_deposit_wallet`, which means that `vault_deposit_wallet` holds both the funds and the fees.
+
+```rust
+  fn transfer_token_ctx(&self) -> CpiContext<'_, '_, '_, 'info, Transfer<'info>> {
+        let cpi_accounts = Transfer {
+            from: self.vault_deposit_wallet.to_account_info(),
+            to: self.user_deposit_wallet.to_account_info(),
+            authority: self.vault_authority.to_account_info(),
+        };
+        let cpi_program = self.token_program.to_account_info();
+        CpiContext::new(cpi_program, cpi_accounts)
+    }
+```
+
+The fees is left in the `vault_deposit_wallet` account, but it is not stored as a value.
+
+`vault_deposit_wallet` account owner will not know the actual amount of fees being collected.
+
+### Internal pre-conditions
+
+_No response_
+
+### External pre-conditions
+
+_No response_
+
+### Attack Path
+
+_No response_
+
+### Impact
+
+Fees are not calculated and tabulated, the owner will not know how much fees is collected at any point in time.
+
+### PoC
+
+_No response_
+
+### Mitigation
+
+Have another variable that tracks the fees amount for every message received.
+