Uneven Gingham Locust - State changes are overwritten during anchor serialization when two accounts are the same

[sherlock-admin3]

Uneven Gingham Locust

High

State changes are overwritten during anchor serialization when two accounts are the same

Summary

The swap operation takes in 3 pool accounts:

1. woopool_from
2. woopool_to
3. woopool_quote

Case 1:
In Base-to-Quote Swap, woopool_from is base token pool. woopool_to, woopool_quote will be the quote token pool and both are same accounts woopool_to == woopool_quote.

Case 2:
In Quote-to-Base Swap, woopool_to is base token pool. woopool_from, woopool_quote will be the quote token pool and both are same accounts woopool_from == woopool_quote.

Case 3:
In the case of Base-to-Base all three pools will be different.

In case 1 and case 2, two of the passed-in accounts are same. Because of how anchor works, at the end of the execution, changes of only one pool will be saved. For example in case 1 when woopool_to, woopool_quote are same, at the end of the program execution, updates to either woopool_to or woopool_quote are recorded.

Anchor #[derive(Accounts)] macro works by creating a in-memory copy of the account data, modifying the memory and then writing back into the account data:

• For each of the account, Anchor performs required checks based on the type Account<'info, WooConfig>, Program<'info, Token>, etc.
• For Account<'a, T> type accounts, Anchor deserializes the AccountInfo.data into T. For e.g, if account X is passed for Account<'a, WooPool> then Anchor deserializes X.data into type WooPool and keeps the copy in memory.
  • Same as memory copies of state variables in Solidity.
• The deserialized in-memory copies of the accounts are passed to the instruction-handler: for example swap function is an instruction handler.
• After the instruction handler finishes execution, Anchor serializes the accounts and writes into the account data.
• The account data is persistent and hence the state changes are saved.

Consider a Solidity function which copies a struct state variable into memory, changes the values in memory and at the end of the function copies the memory values into the state variable. Anchor does the same thing.

This leads to a storage overwrite issue when two of the passed-in accounts are same. When the second struct written into account data, it will overwrite any changes that are present in the first struct.

The swap instruction handler performs the following updates to the pools:

1. Add from_amount to woopool_from reserve
2. Subtract to_amount from woopool_to reserve
3. Subtract swap_fee from woopool_quote reserve
4. Add swap_fee to unclaimed fee

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L189-L194

woopool_quote is serialized last hence it will overwrite any previous changes if they are same accounts.

• If woopool_from == woopool_quote:
  • Update in 1 will be overwritten by 3 and 4.
  • from_amount will not be added to woopool_from (quote pool) reserves
• if woopool_to == woopool_quote:
  • Update in 2 will be overwritten by 3 and 4.
  • to_amount will not be deducted from the woopool_to (quote pool) reserves

Root Cause

The swap function tries to handle all type of swaps in a single instruction allowing for cases where two writable accounts are the same leading to storage overwrite issues

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L13-L84

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

User performs a swap: either Base to quote or quote to base. The reserves of the quote will not be updated correctly.

Impact

• If swap sells quote:
  • from_amount will not be added to woopool_from (quote pool) reserves
• if swap sells base for quote:
  • to_amount will not be deducted from the woopool_to (quote pool) reserves

This leads to incorrect accounting of quote pool reserves variable.

1. The reserve will be higher than the quote pool token vault balance Or
2. The reserve will be lower than the quote pool token vault balance

Additional to incorrect state, because balance is checked before transfering tokens in claim_fee, the issue might prevent admin from claiming fees. The issue might have additional implications that aren't noted here.

PoC

In the 1_woofi_swap.ts test file, update the swap_from_sol_to_usdc test to print the reserve values of the toPool before and after the swap.

Add the following at line 265 in tests/1_woofi_swap.ts:

      let toPoolDataBefore = null;
      try {
        toPoolDataBefore = await program.account.wooPool.fetch(toPoolParams.woopool);
      } catch (e) {
          console.log(e);
          console.log("fetch failed");
          return;
      }
      console.log("toPool reserve before swap:" + toPoolDataBefore.reserve);
      console.log("toPool unclaimed fee before swap:" + toPoolDataBefore.unclaimedFee);

and Add the following at line 357 at the end of the that test (after swap is called`:

        let toPoolDataAfter = null;
        try {
          toPoolDataAfter = await program.account.wooPool.fetch(toPoolParams.woopool);
        } catch (e) {
            console.log(e);
            console.log("fetch failed");
            return;
        }
        console.log("toPool reserve after swap:" + toPoolDataAfter.reserve);
        console.log("toPool unclaimed fee after swap:" + toPoolDataAfter.unclaimedFee);

The output will be:

    #swap_between_sol_and_usdc
fromWallet PublicKey:Cybv9pDy9MoysaTk3gkRi3RCtW5gz1jRzZouF47TyfnB
solWalletTokenAccount:GCGi8N26WRcwBedHCFoRo8iycWwLadaNYJx1CW7PyJQ
usdcWalletTokenAccount:Aynux9Ei1FczuPNb5i4Z6cgJisABx3Ty5xNZUfsR6Gst
fromWallet Balance:0
fromTokenAccount amount:1000000
fromTokenAccount decimals:9
toPool reserve before swap:200000
toPool unclaimed fee before swap:0
price - 14597424250
feasible - 1
price - 0
feasible - 0
toAmount:136775
swapFee:4231
toTokenAccount amount:136775
toTokenAccount decimals:6
toPool reserve after swap:195769
toPool unclaimed fee after swap:4231
      ✔ swap_from_sol_to_usdc (3274ms)

In the test, woopool_to == woopool_quote == USDC pool. The reserve of the USDC pool should be

usdcPool.reserve = usdcPool.reserve - toAmount - swapFee = 200000 - 136775 - 4231

However, in the output it can be seen that, the usdcPool.reserve is equal to reserve - swap_fee = 200000 - 4231 = 195769

The deduction of to_amount from the woopool_to.reserve is not present:

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L190

Only the swap_fee deduction is preserved because it was deducted from woopool_quote:

https://github.com/sherlock-audit/2024-08-woofi-solana-deployment/blob/main/WOOFi_Solana/programs/woofi/src/instructions/swap.rs#L193

When Anchor serialized the accounts at the end, the woopool_to is first written to the account data and then the woopool_quote. Because both are same accounts, the changes in woopool_to are overwritten when woopool_quote is serialized and written to account data.

In case of swap usdc to sol, the changes in woopool_from will be overwritten by the woopool_quote.

Mitigation

Divide the swap functions into individual sell_base, sell_quote, and sell_base_to_base functions.