State changes are overwritten during anchor serialization when two accounts are the same
Summary
The swap operation takes in 3 pool accounts:
woopool_from
woopool_to
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:
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
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.
The reserve will be higher than the quote pool token vault balance Or
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:
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.
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:
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 eitherwoopool_to
orwoopool_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:Account<'info, WooConfig>
,Program<'info, Token>
, etc.Account<'a, T>
type accounts, Anchor deserializes theAccountInfo.data
intoT
. For e.g, if account X is passed forAccount<'a, WooPool>
then Anchor deserializesX.data
into typeWooPool
and keeps the copy in memory.memory
copies of state variables in Solidity.swap
function is an instruction handler.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 accountdata
, it will overwrite any changes that are present in the first struct.The
swap
instruction handler performs the following updates to the pools:from_amount
towoopool_from
reserveto_amount
fromwoopool_to
reserveswap_fee
fromwoopool_quote
reserveswap_fee
to unclaimed feehttps://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.woopool_from
==woopool_quote
:1
will be overwritten by3
and4
.from_amount
will not be added towoopool_from
(quote pool) reserveswoopool_to
==woopool_quote
:2
will be overwritten by3
and4
.to_amount
will not be deducted from thewoopool_to
(quote pool) reservesRoot 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 tostorage overwrite
issueshttps://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
from_amount
will not be added towoopool_from
(quote pool) reservesto_amount
will not be deducted from thewoopool_to
(quote pool) reservesThis leads to incorrect accounting of
quote pool
reserves variable.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 theswap_from_sol_to_usdc
test to print the reserve values of thetoPool
before and after the swap.Add the following at line 265 in
tests/1_woofi_swap.ts
:and Add the following at line 357 at the end of the that test (after
swap
is called`:The output will be:
In the test,
woopool_to
==woopool_quote
==USDC pool
. Thereserve
of the USDC pool should beHowever, in the output it can be seen that, the
usdcPool.reserve
is equal toreserve - swap_fee = 200000 - 4231 = 195769
The deduction of
to_amount
from thewoopool_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 fromwoopool_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 thewoopool_quote
. Because both are same accounts, the changes inwoopool_to
are overwritten whenwoopool_quote
is serialized and written to account data.In case of swap usdc to sol, the changes in
woopool_from
will be overwritten by thewoopool_quote
.Mitigation
Divide the
swap
functions into individualsell_base
,sell_quote
, andsell_base_to_base
functions.