| sponsor | slug | date | title | findings | contest |
|---|---|---|---|---|---|
Superposition |
2024-08-superposition |
2024-10-18 |
Superposition |
428 |
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the Superposition smart contract system written in Solidity and Rust. The audit took place between August 23 — September 13, 2024.
51 Wardens contributed reports to Superposition:
- DadeKuma
- oakcobalt
- Testerbot
- prapandey031
- zhaojohnson
- wasm_it (0xrex and rustguy)
- adeolu
- 13u9
- ZanyBonzy
- Tricko
- SBSecurity (Slavcheww and Blckhv)
- shaflow2
- Q7
- Silvermist
- mashbust (gulDozer, usmanatique, abdulsamijay and 0xabdullah0)
- eta
- d4r3d3v1l
- peanuts
- aldarion
- pipidu83
- 0xhashiman
- Nikki
- nnez
- OMEN
- nslavchev
- Japy69
- devival
- SpicyMeatball
- IzuMan
- Shubham
- ABAIKUNANBAEV
- Rhaydden
- rare_one
- debo
- Tigerfrake
- 0xAleko
- 0xastronatey
- Agontuk
- swapnaliss
- Sparrow
- NexusAudits (cheatc0d3 and Zanna)
- Bauchibred
- Decap
- dreamcoder
- zhaojie
This audit was judged by 0xsomeone.
Final report assembled by thebrittfactor.
The C4 analysis yielded an aggregated total of 19 unique vulnerabilities. Of these vulnerabilities, 7 received a risk rating in the category of HIGH severity and 12 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 16 reports detailing issues with a risk rating of LOW severity or non-critical.
All of the issues presented here are linked back to their original finding.
The code under review can be found within the C4 Superposition repository, and is composed of 26 smart contracts written in the Solidity programming language and includes 5248 lines of Solidity code.
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
Submitted by ABAIKUNANBAEV, also found by Q7, DadeKuma, ZanyBonzy, Rhaydden, Nikki, nslavchev, Testerbot, eta, d4r3d3v1l, prapandey031, zhaojohnson, oakcobalt, wasm_it, and shaflow2
Inside of lib.rs, there is a function update_emergency_council_7_D_0_C_1_C_58() that is needed to update the emergency council that can disable the pools. However, in the current implementation, nft_manager is updated instead.
This is the current functionality of update_emergency_council_7_D_0_C_1_C_58():
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L1111-1124
pub fn update_emergency_council_7_D_0_C_1_C_58(
&mut self,
manager: Address,
) -> Result<(), Revert> {
assert_eq_or!(
msg::sender(),
self.seawater_admin.get(),
Error::SeawaterAdminOnly
);
self.nft_manager.set(manager);
Ok(())
}
As you can see, the function updates nft_manager contract instead of emergency_council that is needed to be updated. Above this function there is another function that updates nft_manager:
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L1097-1107
pub fn update_nft_manager_9_B_D_F_41_F_6(&mut self, manager: Address) -> Result<(), Revert> {
assert_eq_or!(
msg::sender(),
self.seawater_admin.get(),
Error::SeawaterAdminOnly
);
self.nft_manager.set(manager);
Ok(())
}
As you can see here, in both of the functions nft_manager is updated which is an unexpected behavior and the contract cannot update the emergency_council that handles emergency situations:
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L117-118
// address that's able to activate and disable emergency mode functionality
emergency_council: StorageAddress,
Change update_emergency_council_7_D_0_C_1_C_58() to update emergency_council.
af-afk (Superposition) confirmed via duplicate issue #64
The Warden and its duplicates have correctly identified that the mechanism exposed for updating the
emergency_councilwill incorrectly update thenft_managerinstead.I initially wished to retain a medium risk severity rating for this vulnerability due to how the
emergency_councilis configured during the contract's initialization and its value changing being considered a rare event; however, a different highly sensitive variable is altered instead incorrectly (nft_manager) which would have significant consequences to the system temporarily.Based on the above, I believe that a high-risk rating is appropriate due to the unexpected effects invocation of the function would result in.
Submitted by Japy69, also found by zhaojohnson, ZanyBonzy, Testerbot, IzuMan, Shubham, SBSecurity, Nikki, SpicyMeatball, and oakcobalt
The vulnerability arises from the fact that after a token transfer, the approval status for the token is not revoked. Specifically, the getApproved[_tokenId] is not updated on transfer. This allows the previous owner (or any approved address) to reclaim the NFT by using the approval mechanism to re-transfer the token back to themselves. This is critical because the new owner of the NFT may lose their asset without realizing it, leading to potential exploitation, loss of assets, and decreased trust in the platform.
In the provided approve function, any user can approve themselves or another address for a specific token ID:
/// @inheritdoc IERC721Metadata
function approve(address _approved, uint256 _tokenId) external payable {
_requireAuthorised(msg.sender, _tokenId);
getApproved[_tokenId] = _approved;
}Since the approval is not revoked upon transfer, the previous owner retains the ability to re-transfer the NFT. The _requireAuthorised function is the only check on transfer permission:
function _requireAuthorised(address _from, uint256 _tokenId) internal view {
// revert if the sender is not authorised or the owner
bool isAllowed =
msg.sender == _from ||
isApprovedForAll[_from][msg.sender] ||
msg.sender == getApproved[_tokenId];
require(isAllowed, "not allowed");
require(ownerOf(_tokenId) == _from, "_from is not the owner!");
}- Initial Approval: The owner of a token (
owner1) approves an address (addr2) to transfer their token. - Token Transfer: The token is transferred from
owner1tonewOwner. - Approval Not Revoked: The approval for
addr2is not revoked after the transfer. - NFT Recovery:
addr2can still use their approval to transfer the NFT back to themselves, effectively recovering the NFT fromnewOwner.
There is no existing tests for this specific smart contract (because it is in solidity). A coded POC for this easy-to-understand vulnerability would involve to create all deployment logic.
To prevent this vulnerability, any existing approvals should be revoked when a token is transferred. This can be achieved by adding a line in the transfer function to clear the approval:
getApproved[_tokenId] = address(0);This line should be added to the token transfer function to ensure that any previously approved addresses cannot transfer the NFT after it has been sold or transferred to a new owner.
Token-Transfer
af-afk (Superposition) confirmed via duplicate issue #56
The submission details how approvals are not cleared whenever an NFT transfer occurs, permitting the NFT to be recovered after it has been transferred which breaks a crucial invariant of NFTs and would affect any and all integrations of the NFT (i.e., staking systems, marketplaces, etc.). As such, I believe a high-risk severity rating is appropriate.
Submitted by Q7, also found by aldarion, Nikki, prapandey031, SBSecurity, DadeKuma, nnez, Silvermist, oakcobalt, shaflow2, ABAIKUNANBAEV, nslavchev, rare_one, OMEN, Testerbot, wasm_it, and devival
The current implementation does not perform a check for lower < upper during mint_position, while many other functions assume lower < upper. This discrepancy allows malicious actors to exploit inconsistencies in handling undefined behavior in other parts of the code for their benefit.
- Case when
lower = upper: In theStorageTicks::updatefunction, liquidity can be added without issue because only one boundary and the current tick need to be passed, meaning it should function correctly even if the current tick equals the boundary. However, when calculating the amount of tokens required from the user, due tolower = upper, the calculation can only fall into the first and third branches. Here,sqrt_ratio_a_x_96 = sqrt_ratio_a_x_96, and the difference between these two values multiplies the result, leading to a token amount of 0 regardless of liquidity. A malicious user can exploit this implementation discrepancy to open a position with arbitrary liquidity value without consuming any tokens.
/* pkg/seawater/src/pool.rs */
170 | // calculate liquidity change and the amount of each token we need
171 | if delta != 0 {
172 | let (amount_0, amount_1) = if self.cur_tick.get().sys() < lower {
...
183 |
184 | // we're below the range, we need to move right, we'll need more token0
185 | (
186 | sqrt_price_math::get_amount_0_delta(
187 | tick_math::get_sqrt_ratio_at_tick(lower)?,
188 | tick_math::get_sqrt_ratio_at_tick(upper)?,
189 | delta,
190 | )?,
191 | I256::zero(),
192 | )
193 | } else if self.cur_tick.get().sys() < upper {
194 | // we're inside the range, the liquidity is active and we need both tokens
...
224 | } else {
...
236 | // we're above the range, we need to move left, we'll need token1
237 | (
238 | I256::zero(),
239 | sqrt_price_math::get_amount_1_delta(
240 | tick_math::get_sqrt_ratio_at_tick(lower)?,
241 | tick_math::get_sqrt_ratio_at_tick(upper)?,
242 | delta,
243 | )?,
244 | )
245 | };While the attacker cannot directly profit by closing the position, these positions affect fee calculations. For example, in the provided PoC, the attacker can create a large number of empty positions within their liquidity range to collect significant fees from swapping users, inflating what would normally be a fee of 6 to 1605.
- Case when
lower > upper: Unlike the previous case, whenlower > upperit is possible to create a position with both liquidity and balance. However, the potential issue arises when calling theget_fee_growth_insidefunction to calculate fees. This function also assumeslower < upperin its calculations, and passinglower > upperresults in incorrect fee calculations, allowing the attacker to steal fees from other liquidity providers. However, the current implementation ofget_fee_growth_insidehas a bug where it does not correctly handle overflow as per the Uniswap V3 specification, rendering this exploit temporarily unusable.
#[test]
fn lower_upper_eq_poc() {
test_utils::with_storage::<_, Pools, _>(
Some(address!("3f1Eae7D46d88F08fc2F8ed27FCb2AB183EB2d0E").into_array()), // sender
None,
None,
None,
|contract| -> Result<(), Vec<u8>> {
let token0 = address!("9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0");
contract.ctor(msg::sender(), Address::ZERO, Address::ZERO)?;
contract.create_pool_D650_E2_D0(
token0,
U256::from_limbs([0, 42942782960, 0, 0]),
500, // fee
10, // tick spacing
u128::MAX,
)?;
contract.enable_pool_579_D_A658(token0, true)?;
contract.mint_position_B_C5_B086_D(token0, 30000, 50000)?; // Init a normal position
println!("Create normal position: {:?}", contract.update_position_C_7_F_1_F_740(token0, U256::from(0), 50000).unwrap());
println!("Tick before swap {}", contract.cur_tick181_C6_F_D9(token0).unwrap());
// Create 500 malicious empty positions
for i in 0..2000 {
contract.mint_position_B_C5_B086_D(token0, 30000+i*10, 30000+i*10)?;
let (amount0, amount1) = contract.update_position_C_7_F_1_F_740(token0, U256::from(1+i), 100000000000).unwrap();
assert!(amount0 == I256::ZERO && amount1 == I256::ZERO); // Attacker don't need any token
}
println!("Victim swap: {:?}", contract.swap_904369_B_E(
token0,
true,
I256::try_from(10000_i32).unwrap(),
U256::MAX,
).unwrap());
println!("Tick after swap {}", contract.cur_tick181_C6_F_D9(token0).unwrap());
println!("Refresh position {:?}", contract.update_position_C_7_F_1_F_740(token0, U256::from(0), 0).unwrap());
println!("Fee owed: {:?}", contract.fees_owed_22_F28_D_B_D(token0, U256::from(0)).unwrap());
Ok(())
},
)
.unwrap()
}Add assert_or!(lower < upper, Error::InvalidTick) check.
Invalid Validation
af-afk (Superposition) confirmed via duplicate issue #59
The Warden has properly identified that the creation of a position does not sufficiently validate its lower and upper tick values, permitting positions whereby the ticks are inverted or equal to be created with significant consequences.
I consider a high-risk severity rating appropriate and have penalized submissions that simply identified the vulnerability (i.e., as a medium, or simply noted it with no impact justification) by 25% (i.e., rewarded with a partial reward of 75%).
[H-04] Position's owed fees should allow underflow but it reverts instead, resulting in locked funds
Submitted by DadeKuma, also found by Tricko and SBSecurity
The math used to calculate how many fees are owed to the user does not allow underflows, but this is a necessary due to how the Uniswap logic works as fees can be negative. This impact adding/removing funds to a position, resulting in permanently locked funds due to a revert.
Similar to Position's fee growth can revert resulting in funds permanently locked but with a different function/root cause, both issues must be fixed separately.
In position.update underflows are not allowed:
pub fn update(
&mut self,
id: U256,
delta: i128,
fee_growth_inside_0: U256,
fee_growth_inside_1: U256,
) -> Result<(), Error> {
let mut info = self.positions.setter(id);
let owed_fees_0 = full_math::mul_div(
fee_growth_inside_0
-> .checked_sub(info.fee_growth_inside_0.get())
.ok_or(Error::FeeGrowthSubPos)?,
U256::from(info.liquidity.get()),
full_math::Q128,
)?;
let owed_fees_1 = full_math::mul_div(
fee_growth_inside_1
-> .checked_sub(info.fee_growth_inside_1.get())
.ok_or(Error::FeeGrowthSubPos)?,
U256::from(info.liquidity.get()),
full_math::Q128,
)?;
let liquidity_next = liquidity_math::add_delta(info.liquidity.get().sys(), delta)?;
if delta != 0 {
info.liquidity.set(U128::lib(&liquidity_next));
}
info.fee_growth_inside_0.set(fee_growth_inside_0);
info.fee_growth_inside_1.set(fee_growth_inside_1);
if !owed_fees_0.is_zero() {
// overflow is the user's problem, they should withdraw earlier
let new_fees_0 = info
.token_owed_0
.get()
.wrapping_add(U128::wrapping_from(owed_fees_0));
info.token_owed_0.set(new_fees_0);
}
if !owed_fees_1.is_zero() {
let new_fees_1 = info
.token_owed_1
.get()
.wrapping_add(U128::wrapping_from(owed_fees_1));
info.token_owed_1.set(new_fees_1);
}
Ok(())
}While the original Uniswap version allows underflows:
uint128 tokensOwed0 =
uint128(
FullMath.mulDiv(
feeGrowthInside0X128 - _self.feeGrowthInside0LastX128,
_self.liquidity,
FixedPoint128.Q128
)
);
uint128 tokensOwed1 =
uint128(
FullMath.mulDiv(
feeGrowthInside1X128 - _self.feeGrowthInside1LastX128,
_self.liquidity,
FixedPoint128.Q128
)
);The issue is that negative fees are expected due to how the formula works. It is explained in detail in this Uniswap's issue here.
This function is used every time a position is updated, so it will be impossible to remove funds from it when the underflow happens because it will revert, resulting in locked funds.
Consider using - to let Rust underflow without errors in release mode:
let owed_fees_0 = full_math::mul_div(
fee_growth_inside_0
- .checked_sub(info.fee_growth_inside_0.get())
- .ok_or(Error::FeeGrowthSubPos)?,
+ - info.fee_growth_inside_0.get()
U256::from(info.liquidity.get()),
full_math::Q128,
)?;
let owed_fees_1 = full_math::mul_div(
fee_growth_inside_1
- .checked_sub(info.fee_growth_inside_1.get())
- .ok_or(Error::FeeGrowthSubPos)?,
+ - info.fee_growth_inside_1.get()
U256::from(info.liquidity.get()),
full_math::Q128,
)?;Alternatively, consider using wrapping_sub.
Uniswap
The submission and its duplicates properly identify that the
position.rsfile fails to permit underflows to occur when calculating fees which might result in inaccessible funds. I have personally observed this vulnerability in production, and I am inclined to agree with a high-risk severity rating as positions resulting in the underflow would effectively become permanently inaccessible.
af-afk (Superposition) confirmed
Submitted by mashbust, also found by Q7, eta, d4r3d3v1l, DadeKuma, Tricko, and shaflow2
A critical bug has been identified in the collect_protocol_7540_F_A_9_F function of the Seawater Automated Market Maker (AMM) protocol. This vulnerability affects the core functionality of fee collection, rendering the system unable to transfer protocol fees from liquidity pools to the Seawater Admin or any designated recipient. The bug stems from incorrect parameter handling during the invocation of the transfer_to_addr function, which results in a complete failure of the fee withdrawal process.
The impact of this issue is severe:
-
Denial of Service (DoS): The system's inability to transfer protocol fees halts the entire fee collection process, effectively disabling this core feature. Without this function, the Seawater Admin cannot retrieve fees collected from liquidity pools, meaning the protocol cannot generate or distribute earnings from the token swapping activity in these pools.
-
Financial Loss: Since the protocol relies on fees for its economic model, this bug causes a direct financial loss. The protocol's earnings from fees generated by liquidity pools are locked and inaccessible, leading to loss of funds. Over time, the fees accumulate in the pools, but they cannot be withdrawn by the Seawater Admin or any authorized recipient.
-
Erosion of User Trust: Protocol participants expect proper fee collection and distribution. If this feature fails, users may lose confidence in the protocol's reliability, leading to reduced engagement or participation. For liquidity providers and investors, this issue directly affects expected returns, as fees cannot be withdrawn.
-
Long-term Protocol Viability: If not addressed, this bug could harm the protocol’s long-term viability. Since the protocol is unable to collect revenue from its own pools, its ability to cover operational costs or distribute rewards is compromised. This could lead to a shutdown or significant degradation of the protocol’s functionality.
In summary, the failure to collect protocol fees undermines the core value proposition of the AMM protocol, resulting in a loss of funds, DoS of essential functionality, and the potential collapse of the protocol’s economic model if left unresolved.
The bug exists in the fee collection logic of the collect_protocol_7540_F_A_9_F function located in lib.rs. This function is responsible for transferring collected protocol fees (in the form of tokens) from an AMM pool to the Seawater Admin or another recipient. However, incorrect parameter handling within the transfer_to_addr function causes the entire process to fail.
Here’s a detailed breakdown of the problematic code:
pub fn collect_protocol_7540_F_A_9_F(
&mut self,
pool: Address,
amount_0: u128,
amount_1: u128,
recipient: Address,
) -> Result<(u128, u128), Revert> {
assert_eq_or!(
msg::sender(),
self.seawater_admin.get(),
Error::SeawaterAdminOnly
);
let (token_0, token_1) = self
.pools
.setter(pool)
.collect_protocol(amount_0, amount_1)?;
// Incorrect transfer logic
erc20::transfer_to_addr(recipient, pool, U256::from(token_0))?;
erc20::transfer_to_addr(recipient, FUSDC_ADDR, U256::from(token_1))?;
#[cfg(feature = "log-events")]
evm::log(events::CollectProtocolFees {
pool,
to: recipient,
amount0: token_0,
amount1: token_1,
});
// transfer tokens
Ok((token_0, token_1))
}The issue arises in the following lines:
erc20::transfer_to_addr(recipient, pool, U256::from(token_0))?;
erc20::transfer_to_addr(recipient, FUSDC_ADDR, U256::from(token_1))?;In this block:
recipientis incorrectly passed as the token address.poolorFUSDC_ADDRis incorrectly passed as the recipient.
This order causes the transfer_to_addr function to fail because it expects the first parameter to be the token address and the second parameter to be the recipient address. Here's the correct signature of the transfer_to_addr function from wasm_erc20.rs:
pub fn transfer_to_addr(token: Address, recipient: Address, amount: U256) -> Result<(), Error> {
safe_transfer(token, recipient, amount)
}Since the parameters are passed in the wrong order, the transfer_to_addr function fails, leading to a complete halt of the fee transfer process. As a result, the collected fees cannot be moved from the pools to the Seawater Admin or recipient, effectively blocking the protocol’s ability to distribute or utilize these funds.
- Call the
collect_protocol_7540_F_A_9_Ffunction with a valid pool address, token amounts, and recipient address. - The function will attempt to transfer the protocol fees using
erc20::transfer_to_addr. - Since the arguments are incorrectly passed (with the recipient and token address swapped), the transaction will fail, and no tokens will be transferred.
Failed Transaction Log:
[18881] collect_protocol_7540_F_A_9_F::collect_fees()
├─ [11555] erc20::transfer_to_addr(token: [recipient], pool: [token], 1)
│ └─ ← [Revert]
└─ ← [Revert]
This log highlights the issue that the transfer_to_addr function is attempting to use the recipient address as the token address, and the pool address as the recipient. This mismatch causes the transfer to fail, preventing the collected protocol fees from being withdrawn.
- Rust Analyzer: Employed to debug and trace the logic flow of the contract code, helping to identify the parameter mismatch issue.
- test-node: Utilized to track failed transactions and confirm the DoS condition caused by the bug.
- Visual Studio Code (VSCode): Main code editor used to examine and analyze the source code of the protocol.
To fix this issue, the arguments passed to the erc20::transfer_to_addr function need to be correctly ordered, ensuring that the token address is provided first, followed by the recipient address. This correction will allow the transfer of protocol fees from the liquidity pools to the designated recipient.
Corrected Code:
erc20::transfer_to_addr(pool, recipient, U256::from(token_0))?;
erc20::transfer_to_addr(FUSDC_ADDR, recipient, U256::from(token_1))?;This update reverses the order of parameters, passing the token address as the first parameter and the recipient address as the second parameter, as expected by the transfer_to_addr function. By implementing this fix, the protocol will correctly transfer collected fees, ensuring that the Seawater Admin or any authorized recipient can successfully withdraw fees from the AMM pools.
-
Add Unit Tests: To prevent this issue from recurring, add specific unit tests to validate the correct behavior of the
collect_protocol_7540_F_A_9_Ffunction. These tests should confirm that fees can be collected and transferred without errors. -
Error Handling Improvements: Consider adding more explicit error messages to the
transfer_to_addrfunction to help identify issues during fee transfers. This could include checking whether the token address and recipient address are valid before proceeding with the transfer. -
Gas Efficiency: After resolving this issue, it’s advisable to audit the overall gas usage of the fee collection process to ensure that no unnecessary operations are being performed during the transfer of protocol fees.
DoS
af-afk (Superposition) confirmed via duplicate issue #60
The Warden has identified that an incorrect order of arguments in the EIP-20 transfer calls within the
lib::collect_protocol_7540_F_A_9_Ffunction will cause a token transfer to be attempted with therecipientaddress as the "token" thereby failing on each invocation.I believe a high severity rating is appropriate given that funds are directly lost and are irrecoverable due to an improper implementation of the protocol fee claim mechanism.
Submitted by SBSecurity, also found by DadeKuma, ZanyBonzy, aldarion, peanuts, 0xhashiman, Tricko, and Q7
When operations need to calculate the Superposition position's fee growth, it uses a similar function implemented by uniswap v3.
However, according to this known issue, Uniswap/v3-core#573, the contract implicitly relies on underflow/overflow when calculating the fee growth if underflow is prevented, some operations that depend on fee growth will revert.
It can be observed that the current implementation of tick::get_fee_growth_inside does not allow underflow/overflow to happen when calculating fee_growth_inside_0 and fee_growth_inside_1 because the contract uses checked math with additional overflow/underflow checks; which will perform subtraction that returns none instead of wrapping around on underflow in tick.rs.
pub fn get_fee_growth_inside(
&mut self,
lower_tick: i32,
upper_tick: i32,
cur_tick: i32,
fee_growth_global_0: &U256,
fee_growth_global_1: &U256,
) -> Result<(U256, U256), Error> {
// the fee growth inside this tick is the total fee
// growth, minus the fee growth outside this tick
let lower = self.ticks.get(lower_tick);
let upper = self.ticks.get(upper_tick);
let (fee_growth_below_0, fee_growth_below_1) = if cur_tick >= lower_tick {
#[cfg(feature = "testing-dbg")]
dbg!((
"cur_tick >= lower_tick",
current_test!(),
lower.fee_growth_outside_0.get().to_string(),
lower.fee_growth_outside_1.get().to_string()
));
(
lower.fee_growth_outside_0.get(),
lower.fee_growth_outside_1.get(),
)
} else {
#[cfg(feature = "testing-dbg")]
dbg!((
"cur_tick < lower_tick",
current_test!(),
fee_growth_global_0,
fee_growth_global_1,
lower.fee_growth_outside_0.get().to_string(),
lower.fee_growth_outside_1.get().to_string()
));
(
fee_growth_global_0
.checked_sub(lower.fee_growth_outside_0.get())
.ok_or(Error::FeeGrowthSubTick)?,
fee_growth_global_1
.checked_sub(lower.fee_growth_outside_1.get())
.ok_or(Error::FeeGrowthSubTick)?,
)
};
let (fee_growth_above_0, fee_growth_above_1) = if cur_tick < upper_tick {
#[cfg(feature = "testing-dbg")]
dbg!((
"cur_tick < upper_tick",
current_test!(),
upper.fee_growth_outside_0.get().to_string(),
upper.fee_growth_outside_1.get().to_string()
));
(
upper.fee_growth_outside_0.get(),
upper.fee_growth_outside_1.get(),
)
} else {
#[cfg(feature = "testing-dbg")]
dbg!((
"cur_tick >= upper_tick",
current_test!(),
fee_growth_global_0,
fee_growth_global_1,
upper.fee_growth_outside_0.get(),
upper.fee_growth_outside_1.get()
));
(
fee_growth_global_0
.checked_sub(upper.fee_growth_outside_0.get())
.ok_or(Error::FeeGrowthSubTick)?,
fee_growth_global_1
.checked_sub(upper.fee_growth_outside_1.get())
.ok_or(Error::FeeGrowthSubTick)?,
)
};
#[cfg(feature = "testing-dbg")] // REMOVEME
{
if *fee_growth_global_0 < fee_growth_below_0 {
dbg!((
"fee_growth_global_0 < fee_growth_below_0",
current_test!(),
fee_growth_global_0.to_string(),
fee_growth_below_0.to_string()
));
}
let fee_growth_global_0 = fee_growth_global_0.checked_sub(fee_growth_below_0).unwrap();
if fee_growth_global_0 < fee_growth_above_0 {
dbg!((
"fee_growth_global_0 < fee_growth_above_0",
current_test!(),
fee_growth_global_0.to_string(),
fee_growth_above_0.to_string()
));
}
}
#[cfg(feature = "testing-dbg")]
dbg!((
"final stage checked sub below",
current_test!(),
fee_growth_global_0
.checked_sub(fee_growth_below_0)
.and_then(|x| x.checked_sub(fee_growth_above_0))
));
Ok((
fee_growth_global_0
.checked_sub(fee_growth_below_0)
.and_then(|x| x.checked_sub(fee_growth_above_0))
.ok_or(Error::FeeGrowthSubTick)?,
fee_growth_global_1
.checked_sub(fee_growth_below_1)
.and_then(|x| x.checked_sub(fee_growth_above_1))
.ok_or(Error::FeeGrowthSubTick)?,
))
}Especially the last lines where we have fee_growth_global_0 - fee_growth_below_0 - fee_growth_above_0 and fee_growth_global_1 - fee_growth_below_1 - fee_growth_above_1.
In case either one of the calculation underflows we will receive Error::FeeGrowthSubTick instead of being wrapped and continue working.
Uniswap allows such underflows to happen because PositionLibrary works with Solidity version < 0.8. Otherwise, this will impact crucial operations that rely on this call, such as liquidation, and will revert unexpectedly. This behavior is quite often, especially for pools that use lower fees.
Furthermore, one of the Main Invariants in the README mentioned that Superposition should follow the UniswapV3 math faithfully, which is not the case here, violating the invariant.
https://github.com/code-423n4/2024-08-superposition?tab=readme-ov-file#main-invariants
H-04 from Particle Leverage AMM.
Use unsafe/unchecked math when calculating the fee growths.
Under/Overflow
af-afk (Superposition) confirmed
The submission and its duplicates properly identify that the
tick.rsfile fails to permit underflows to occur when calculating fees which might result in inaccessible funds. I have personally observed this vulnerability in production, and I am inclined to agree with a high-risk severity rating as positions resulting in the underflow would effectively become permanently inaccessible.
Submitted by oakcobalt, also found by Testerbot
In SeawaterAMM.sol, swapOut5E08A399 and swapOutPermit23273373B are intended to allow usdc(token1) -> pool(token0) swap with slippage check. See ISeawaterAMM's doc.
However, both functions have incorrect slippage checks. We see in swapOut5E08A399 swapAmountOut is used to check with minOut. But swapAmountOut is actually usdc(token1), not the output token(token0). This uses an incorrect variable to check slippage.
function swapOut5E08A399(
address token,
uint256 amountIn, //@audit-info note: this is usdc(token1)
uint256 minOut
) external returns (int256, int256) {
(bool success, bytes memory data) = _getExecutorSwap().delegatecall(
abi.encodeCall(
ISeawaterExecutorSwap.swap904369BE,
(token, false, int256(amountIn), type(uint256).max)
)
);
require(success, string(data));
//@audit-info note: swapAmountIn <=> token0 , swapAmountOut <=> token1
|> (int256 swapAmountIn, int256 swapAmountOut) = abi.decode(
data,
(int256, int256)
);
//@audit This should use token0 value, not token1
|> require(swapAmountOut >= int256(minOut), "min out not reached!");
return (swapAmountIn, swapAmountOut);
}For reference, in the swap facet, swap_internal called in the flow returns Ok((amount_0, amount_1)). This means swapAmountOut refers to token1, the input token amount.
//pkg/seawater/src/lib.rs
pub fn swap_internal(
pools: &mut Pools,
pool: Address,
zero_for_one: bool,
amount: I256,
price_limit_x96: U256,
permit2: Option<Permit2Args>,
) -> Result<(I256, I256), Revert> {
let (amount_0, amount_1, _ending_tick) =
pools
.pools
.setter(pool)
.swap(zero_for_one, amount, price_limit_x96)?;
...
Ok((amount_0, amount_1))swapOutPermit23273373B also has the same erroneous slippage check:
//pkg/sol/SeawaterAMM.sol
function swapOutPermit23273373B(address token, uint256 amountIn, uint256 minOut, uint256 nonce, uint256 deadline, uint256 maxAmount, bytes memory sig) external returns (int256, int256) {
...
|> (int256 swapAmountIn, int256 swapAmountOut) = abi.decode(data, (int256, int256));
|> require(swapAmountOut >= int256(minOut), "min out not reached!");
return (swapAmountIn, swapAmountOut);
}Invalid slippage checks will cause users to lose funds during swaps.
Consider changing to:
...
(int256 swapAmountOut, int256 swapAmountIn) = abi.decode(
data,
(int256, int256)
);
require(uint256(-swapAmountOut) >= minOut, "min out not reached!");
return (swapAmountOut, swapAmountIn);Error
af-afk (Superposition) confirmed via duplicate issue #53
The Warden has identified that the slippage protections of the swap-out functions are incorrect and thus ineffective due to validating an incorrect amount, resulting in either incorrect successful executions or incorrect failed executions depending on the price relation between the two assets.
A medium-risk severity rating is appropriate given that a subset of the system's functionality is affected with slippage being the component affected.
0xsomeone (judge) increased severity to High and commented:
After revisiting this submission in light of the comments shared in Issue #53, I am inclined to upgrade it to a high-risk severity rating as user funds are directly impacted.
Submitted by adeolu, also found by debo (1, 2), Tigerfrake, 0xAleko, 0xastronatey, zhaojohnson, nslavchev, ZanyBonzy, Rhaydden, Nikki, ABAIKUNANBAEV, Agontuk, OMEN, Testerbot, mashbust, swapnaliss, Sparrow, NexusAudits, SBSecurity, Bauchibred, Decap, wasm_it, oakcobalt, SpicyMeatball, and dreamcoder
_onTransferReceived does not work as intended or described in the function natspec.
- It will not revert when the recipient does not implement
onERC721Received()function correctly (does not returnonERC721Received().selector). - It will revert when the recipient implements
onERC721Received()function correctly and as described/specified by the EIP 712 (returnsonERC721Received().selector). - This will prevent transfers to contracts that have correctly implemented the
ERC721TokenReceiverinterface to accept safe token transfers viasafeTransferFrom().
_onTransferReceived() has a require statement that will pass when the recipient does not return the IERC721TokenReceiver.onERC721Received() selector. EIP 721 defines that if a recipient is a contract, it should implement the IERC721TokenReceiver.onERC721Received() function and that function must return the IERC721TokenReceiver.onERC721Received() selector for it to be recognized as a valid erc721 token receiver.
Snippet of the faulty require statement below:
bytes4 data = IERC721TokenReceiver(_to).onERC721Received(
_sender,
_from,
_tokenId,
// this is empty byte data that can be optionally passed to
// the contract we're confirming is able to receive NFTs
""
);
require(
data != IERC721TokenReceiver.onERC721Received.selector,
"bad nft transfer received data"
);We can see that the require statement expects the data returned from the erc721 token receiver to not be equal to IERC721TokenReceiver.onERC721Received.selector. This means that if the recipient implements the onERC721Received function correctly and returns IERC721TokenReceiver.onERC721Received.selector, the require statement in function _onTransferReceived() will revert the whole transfer.
Create foundry test repo and run the file below in test folder:
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import {Test, console} from "forge-std/Test.sol";
/// @dev Note: the ERC-165 identifier for this interface is 0x150b7a02.
interface IERC721TokenReceiver {
/// @notice Handle the receipt of an NFT
/// @dev The ERC721 smart contract calls this function on the recipient
/// after a `transfer`. This function MAY throw to revert and reject the
/// transfer. Return of other than the magic value MUST result in the
/// transaction being reverted.
/// Note: the contract address is always the message sender.
/// @param _operator The address which called `safeTransferFrom` function
/// @param _from The address which previously owned the token
/// @param _tokenId The NFT identifier which is being transferred
/// @param _data Additional data with no specified format
/// @return `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
/// unless throwing
function onERC721Received(
address _operator,
address _from,
uint256 _tokenId,
bytes memory _data
) external returns (bytes4);
}
/*
* OwnershipNFTs is a simple interface for tracking ownership of
* positions in the Seawater Stylus contract.
*/
contract OwnershipNFTs {
/**
* @notice _onTransferReceived by calling the callback `onERC721Received`
* in the recipient if they have codesize > 0. if the callback
* doesn't return the selector, revert!
* @param _sender that did the transfer
* @param _from owner of the NFT that the sender is transferring
* @param _to recipient of the NFT that we're calling the function on
* @param _tokenId that we're transferring from our internal storage
*/
// _onTransferReceived() is exact same function that is in the codebase, with no changes to logic whatsoever
function _onTransferReceived(
address _sender,
address _from,
address _to,
uint256 _tokenId
) internal {
// only call the callback if the receiver is a contract
if (_to.code.length == 0) return;
bytes4 data = IERC721TokenReceiver(_to).onERC721Received(
_sender,
_from,
_tokenId,
// this is empty byte data that can be optionally passed to
// the contract we're confirming is able to receive NFTs
""
);
require(
data != IERC721TokenReceiver.onERC721Received.selector,
"bad nft transfer received data"
);
}
// mock wrapper function that calls the internal _onTransferReceived()
function safeTransferFrom(
address _from,
address _to,
uint256 _tokenId
) external {
_onTransferReceived(msg.sender, _from, _to, _tokenId);
}
}
contract MockCompliantReceiver is IERC721TokenReceiver {
function onERC721Received(
address _operator,
address _from,
uint256 _tokenId,
bytes memory _data
) external returns (bytes4) {
return IERC721TokenReceiver.onERC721Received.selector;
}
}
contract receiverTest is Test {
MockCompliantReceiver compliant_receiver;
OwnershipNFTs ownershipNft;
function setUp() public {
//deploy MockCompliantReceiver
compliant_receiver = new MockCompliantReceiver();
//deploy ownershipNft contract
ownershipNft = new OwnershipNFTs();
}
function test_revertForCompliantReceiver() public {
/* we expect the call to revert because of the faulty require statement */
/** this shows that the _onTransferReceived will always reject transfers to a contract that is
IERC721TokenReceiver compliant. */
vm.expectRevert(bytes("bad nft transfer received data"));
ownershipNft.safeTransferFrom(
msg.sender,
payable(address(compliant_receiver)), //param `to` is set to be the compliant receiver here
1
);
}
}Change the != in the require statement to ==:
require(
data == IERC721TokenReceiver.onERC721Received.selector,
"bad nft transfer received data"
);Context
af-afk (Superposition) confirmed and commented via duplicate issue #55:
We don't believe this is a high risk submission since assets cannot be stolen. Maybe transfers to up to spec contracts could not be used, but that's just frustrating for users.
0xsomeone (judge) decreased severity to Medium and commented:
The submission details how the EIP-721 safe transfers will fail to validate the return data properly, causing EIP-721 callback integrations to fail for the token.
I believe a medium-risk severity rating is better suited for this submission given that assets are not at risk and functionality becomes inaccessible that should normally be circumventable.
Submitted by adeolu
ERC-721 standard has two variations of the safeTransferFrom(), one with no bytes data param and the other with a bytes data param. The second variation that accepts a bytes data param is meant to pass that data param to the token recipient during the IERC721TokenReceiver. onERC721Received() call. This bytes data param is usually an encoding of extra variables which is used by the recipient contract for the extra logic it will perform as it receives a token.
OwnershipNFTs.sol is ERC721 compliant and so it has two safeTransferFrom() functions but the second safeTransferFrom() has a bytes data param which is not passed into the recipient during IERC721TokenReceiver. onERC721Received() call to recipient. It accepts the bytes data param but doesn't use it.
/// @inheritdoc IERC721Metadata
function safeTransferFrom(
address _from,
address _to,
uint256 _tokenId,
bytes calldata /* _data */
) external payable {
_transfer(_from, _to, _tokenId);
_onTransferReceived(msg.sender, _from, _to, _tokenId); As we can see above, the bytes param is declared as an arbitrary param, perhaps just to conform to IERC721 spec but the bytes param is not used in the logic at all, and it ought to be used as defined here in the EIP.
/// @param data Additional data with no specified format, sent in call to
_to
For contracts that integrate OwnershipNFTs or receive transfers, if they have logic which require the additional bytes passed in via the second safeTransferFrom(), they will be unable to execute this logic and may reject the transfers; or revert as this bytes data supplied by the caller of the safeTransferFrom() is not passed into them via the IERC721TokenReceiver.onERC721Received() call. Instead, empty bytes is passed into IERC721TokenReceiver.onERC721Received(), as seen here.
Below is a POC which shows how passing empty bytes by default instead of passing the bytes specified by the sender into a tokenReceiver may cause the transfer to fail.
Run with forge test:
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import {Test, console} from "forge-std/Test.sol";
/// @dev Note: the ERC-165 identifier for this interface is 0x150b7a02.
interface IERC721TokenReceiver {
/// @notice Handle the receipt of an NFT
/// @dev The ERC721 smart contract calls this function on the recipient
/// after a `transfer`. This function MAY throw to revert and reject the
/// transfer. Return of other than the magic value MUST result in the
/// transaction being reverted.
/// Note: the contract address is always the message sender.
/// @param _operator The address which called `safeTransferFrom` function
/// @param _from The address which previously owned the token
/// @param _tokenId The NFT identifier which is being transferred
/// @param _data Additional data with no specified format
/// @return `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
/// unless throwing
function onERC721Received(
address _operator,
address _from,
uint256 _tokenId,
bytes memory _data
) external returns (bytes4);
}
/*
* OwnershipNFTs is a simple interface for tracking ownership of
* positions in the Seawater Stylus contract.
*/
contract OwnershipNFTs {
/**
* @notice _onTransferReceived by calling the callback `onERC721Received`
* in the recipient if they have codesize > 0. if the callback
* doesn't return the selector, revert!
* @param _sender that did the transfer
* @param _from owner of the NFT that the sender is transferring
* @param _to recipient of the NFT that we're calling the function on
* @param _tokenId that we're transferring from our internal storage
*/
// _onTransferReceived() is exact same function that is in the codebase, with no changes to logic whatsoever
function _onTransferReceived(
address _sender,
address _from,
address _to,
uint256 _tokenId
) internal {
// only call the callback if the receiver is a contract
if (_to.code.length == 0) return;
bytes4 data = IERC721TokenReceiver(_to).onERC721Received(
_sender,
_from,
_tokenId,
// this is empty byte data that can be optionally passed to
// the contract we're confirming is able to receive NFTs
""
);
//@audit we don't expect execution to get here, revert should happen in the call to _to because of decoding of empty bytes in _to logic
// require(
// data != IERC721TokenReceiver.onERC721Received.selector,
// "bad nft transfer received data"
// );
}
//the function below is same as seen here -> https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L148-L157
function safeTransferFrom(
address _from,
address _to,
uint256 _tokenId,
bytes calldata /** _data */
) external {
//_transfer(_from, _to, _tokenId);
_onTransferReceived(msg.sender, _from, _to, _tokenId);
}
}
contract MockCompliantReceiver is IERC721TokenReceiver {
function onERC721Received(
address _operator,
address _from,
uint256 _tokenId,
bytes memory _data
) external returns (bytes4) {
//get values in bytes memory _data
(uint timestamp, uint commandID) = abi.decode(_data, (uint, uint));
return IERC721TokenReceiver.onERC721Received.selector;
}
}
contract receiverTest is Test {
MockCompliantReceiver compliant_receiver;
OwnershipNFTs ownershipNft;
function setUp() public {
//deploy MockCompliantReceiver
compliant_receiver = new MockCompliantReceiver();
//deploy ownershipNft contract
ownershipNft = new OwnershipNFTs();
}
function test_revertForCompliantReceiver() public {
//data to be encoded and expected passed into the recipient contract by a sender/user
bytes memory data = abi.encode(block.timestamp, 10);
/* we expect the call to revert because of the decoding of the empty bytes data passed into _to.OnERC721Received() */
vm.expectRevert();
ownershipNft.safeTransferFrom(
msg.sender,
payable(address(compliant_receiver)), //param `to` is set to be the compliant receiver here
1,
data
);
}
}The implementation of the safeTransferFrom() function that accepts additional bytes is not sufficient and not as defined by the EIP-721 spec. The bytes param passed in by a caller is not sent to the recipient contract. Receiving contracts may need to decode this bytes param, but since empty bytes are passed in to recipient by default in OwnershipNfts this will cause reverts/inaccessibility to this function/feature of the ERC721.
The issue stems from the _onTransferReceived() not accepting a bytes data param. Modify it to accept it and pass that param into the IERC721TokenReceiver.onERC721Received() call. Then in each safeTransferFrom(), pass the bytes param into the modified _onTransferReceived(). If the safeTransferFrom() accepts no bytes param, pass empty bytes into _onTransferReceived().
function _onTransferReceived(
address _sender,
address _from,
address _to,
uint256 _tokenId,
bytes calldata data
) internal {
...
bytes4 data = IERC721TokenReceiver(_to).onERC721Received(
_sender,
_from,
_tokenId,
data
);
...
}
/// @inheritdoc IERC721Metadata
function safeTransferFrom(
address _from,
address _to,
uint256 _tokenId
) external payable {
_transfer(_from, _to, _tokenId);
_onTransferReceived(msg.sender, _from, _to, _tokenId, "");
}
/// @inheritdoc IERC721Metadata
function safeTransferFrom(
address _from,
address _to,
uint256 _tokenId,
bytes calldata _data
) external payable {
_transfer(_from, _to, _tokenId);
_onTransferReceived(msg.sender, _from, _to, _tokenId, _data);
}ERC721
0xsomeone (judge) decreased severity to Low
@0xsomeone - I would like implore you to review your decision on this finding as it is quite different from the findings it was grouped with.
This finding does not just talk about EIP-721 compliance but spotlights an implementation flaw in the callback to the token receiver. The hardcoding of the empty bytes passed to the token receiver when the receiver is a contract obstructs the use case of the ERC721Receiver callback/hook . The use cases of this hooks are
- Ensure that a contract is ready to receive the erc721 token.
- Allow for execution of additional custom logic on receipt of the token.
- Allow for sender to communicate with receiving contract via the bytes data that should be passed into it via the callback. This bytes data will then be used in the additional logic to be executed upon the contract receiving the token.
The OwnershipNFT as it is prevents use cases 2 and 3 and in some cases will may prevent 1 because if a contract that has custom logic which requires bytes to be passed in, it will never be ready to receive the token.
Bytes data passed in by a token sender should be propagated to the token receiver contracts. This is the correct, expected and complete implementation of
safeTransferFrom() -> receiver.OnERC721Recevied()callback flow.
0xsomeone (judge) increased severity to Medium and commented:
@adeolu - Indeed, this finding seems to have been grouped incorrectly during the validation phase. Per the rationale laid out in Issue #55, the EIP-721 callback feature was deliberately implemented and does not forward the
_datapayload as described.A medium severity rating for this submission, similar to #55, is appropriate.
af-afk (Superposition) confirmed
Submitted by prapandey031
The protocol provides a function to increment a position's liquidity:
function incrPositionC3AC7CAA(
address pool,
uint256 id,
uint256 amount0Min,
uint256 amount1Min,
uint256 amount0Desired,
uint256 amount1Desired
) external returns (uint256, uint256);It is present in the SeawaterAmm.sol:
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/SeawaterAMM.sol#L464
function incrPositionC3AC7CAA(
address /* pool */,
uint256 /* id */,
uint256 /* amount0Min */,
uint256 /* amount1Min */,
uint256 /* amount0Desired */,
uint256 /* amount1Desired */
) external returns (uint256, uint256) {
directDelegate(_getExecutorUpdatePosition());
}It calls a function in lib.rs:
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L914
#[allow(non_snake_case)]
pub fn incr_position_C_3_A_C_7_C_A_A(
&mut self,
pool: Address,
id: U256,
amount_0_min: U256,
amount_1_min: U256,
amount_0_desired: U256,
amount_1_desired: U256,
) -> Result<(U256, U256), Revert> {
self.adjust_position_internal(
pool,
id,
amount_0_min,
amount_1_min,
amount_0_desired,
amount_1_desired,
false,
None,
)
}This calls an internal function in lib.rs:
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L730
#[allow(clippy::too_many_arguments)]
pub fn adjust_position_internal(
&mut self,
pool: Address,
id: U256,
amount_0_min: U256,
amount_1_min: U256,
amount_0_desired: U256,
amount_1_desired: U256,
giving: bool,
permit2: Option<(Permit2Args, Permit2Args)>,
) -> Result<(U256, U256), Revert> {
assert_eq_or!(
msg::sender(),
self.position_owners.get(id),
Error::PositionOwnerOnly
);
let (amount_0, amount_1) = self.pools.setter(pool).adjust_position(
id,
amount_0_desired,
amount_1_desired,
giving,
)?;
.
.
.
}This function calls a function in pool.rs:
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L253
pub fn adjust_position(
&mut self,
id: U256,
amount_0: U256,
amount_1: U256,
giving: bool,
) -> Result<(I256, I256), Revert> {
// calculate the delta using the amounts that we have here, guaranteeing
// that we don't dip below the amount that's supplied as the minimum.
let position = self.positions.positions.get(id);
let sqrt_ratio_x_96 = tick_math::get_sqrt_ratio_at_tick(self.get_cur_tick().as_i32())?;
let sqrt_ratio_a_x_96 = tick_math::get_sqrt_ratio_at_tick(position.lower.get().as_i32())?;
let sqrt_ratio_b_x_96 = tick_math::get_sqrt_ratio_at_tick(position.upper.get().as_i32())?;
let mut delta = sqrt_price_math::get_liquidity_for_amounts(
sqrt_ratio_x_96, // cur_tick
sqrt_ratio_a_x_96, // lower_tick
sqrt_ratio_b_x_96, // upper_tick
amount_0, // amount_0
amount_1, // amount_1
)?
.to_i128()
.map_or_else(|| Err(Error::LiquidityAmountTooWide), Ok)?;
if giving {
// If we're giving, then we need to take from the delta.
delta = -delta;
}
#[cfg(feature = "testing-dbg")]
dbg!((
"inside adjust_position",
current_test!(),
sqrt_ratio_x_96.to_string(),
sqrt_ratio_a_x_96.to_string(),
sqrt_ratio_b_x_96.to_string(),
amount_0.to_string(),
amount_1.to_string(),
delta
));
// [update_position] should also ensure that we don't do this on a pool that's not currently
// running
self.update_position(id, delta)
}In the above function at LOC-265, we have:
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L265
let sqrt_ratio_x_96 = tick_math::get_sqrt_ratio_at_tick(self.get_cur_tick().as_i32())?;Instead of:
let sqrt_price_x_96 = self.sqrt_price.get();This passes not the current price but the price of the current tick to the get_liquidity_for_amounts() function.
The following is a step-by-step PoC that explains the issue in detail. Let's say there exists a pool with the following configuration:
token0 = X
token1 = FUSDC
There are many available positions; one of them with tick range [0, 5].
Current price of the pool lies between ticks 0 and 1 and is equals to 1.00005
The current tick of a pool is determined by:
Note: please see scenario in warden's original submission.
Now, a user A, owner of the position stated above, calls the function:
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/SeawaterAMM.sol#L464
function incrPositionC3AC7CAA(
address /* pool */,
uint256 /* id */,
uint256 /* amount0Min */,
uint256 /* amount1Min */,
uint256 /* amount0Desired */,
uint256 /* amount1Desired */
) external returns (uint256, uint256) {
directDelegate(_getExecutorUpdatePosition());
}Ultimately, the adjust_position() in pool.rs is called. In this function, at LOC-265, the following is set:
let sqrt_ratio_x_96 = tick_math::get_sqrt_ratio_at_tick(self.get_cur_tick().as_i32())?;This means that sqrt_ratio_x_96 would have 1 (or 2^96) as the curr_tick is 0. Now, get_liquidity_for_amounts() function is called:
pub fn get_liquidity_for_amounts(
sqrt_ratio_x_96: U256,
mut sqrt_ratio_a_x_96: U256,
mut sqrt_ratio_b_x_96: U256,
amount_0: U256,
amount_1: U256,
) -> Result<u128, Error> {
if sqrt_ratio_a_x_96 > sqrt_ratio_b_x_96 {
(sqrt_ratio_a_x_96, sqrt_ratio_b_x_96) = (sqrt_ratio_b_x_96, sqrt_ratio_a_x_96)
};
let delta = if sqrt_ratio_x_96 <= sqrt_ratio_a_x_96 {
get_liquidity_for_amount_0(sqrt_ratio_a_x_96, sqrt_ratio_b_x_96, amount_0)?
} else if sqrt_ratio_x_96 < sqrt_ratio_b_x_96 {
let liq0 = get_liquidity_for_amount_0(sqrt_ratio_x_96, sqrt_ratio_b_x_96, amount_0)?;
let liq1 = get_liquidity_for_amount_1(sqrt_ratio_a_x_96, sqrt_ratio_x_96, amount_1)?;
if liq0 > liq1 {
liq1
} else {
liq0
}
} else {
get_liquidity_for_amount_1(sqrt_ratio_a_x_96, sqrt_ratio_b_x_96, amount_1)?
};
Ok(delta)
}This function checks at LOC-312 that:
if sqrt_ratio_x_96 <= sqrt_ratio_a_x_96and calls:
get_liquidity_for_amount_0(sqrt_ratio_a_x_96, sqrt_ratio_b_x_96, amount_0)?This is wrong as the current price of the pool (= 1.00005) is in the price range of the position ([1, (1.0001)^5]).
Basically, the main liquidity formula used by the protocol currently is:
Note: please see scenario in warden's original submission.
The calculation is also wrong. Let's assume that A calls the function with:
amount0Min = 95% of amount0Desired
amount0Desired = 1000 (assume decimal included)The present calculation would result in an amount0 that would be ~899.98 or 900 which is 90% of amount0Desired and thus, is less than amount0Min. Therefore, this transaction would revert.
The protocol mentions "We should follow Uniswap V3's math faithfully" as one of its core invariants. This must not break. However, as this report explains, this invariant stands broken.
If the amount0Desired and amount0Min (also applicable to amount1) are very close to each other, then the transaction to incrPositionC3AC7CAA() function reverts. So, the severity is High.
Desmos scientific calculator.
Replace the following in the adjust_position() function below:
pub fn adjust_position(
&mut self,
id: U256,
amount_0: U256,
amount_1: U256,
giving: bool,
) -> Result<(I256, I256), Revert> {
// calculate the delta using the amounts that we have here, guaranteeing
// that we don't dip below the amount that's supplied as the minimum.
let position = self.positions.positions.get(id);
- let sqrt_ratio_x_96 = tick_math::get_sqrt_ratio_at_tick(self.get_cur_tick().as_i32())?;
+ let sqrt_ratio_x_96 = self.sqrt_price.get();
let sqrt_ratio_a_x_96 = tick_math::get_sqrt_ratio_at_tick(position.lower.get().as_i32())?;
let sqrt_ratio_b_x_96 = tick_math::get_sqrt_ratio_at_tick(position.upper.get().as_i32())?;
let mut delta = sqrt_price_math::get_liquidity_for_amounts(
sqrt_ratio_x_96, // cur_tick
sqrt_ratio_a_x_96, // lower_tick
sqrt_ratio_b_x_96, // upper_tick
amount_0, // amount_0
amount_1, // amount_1
)?
.to_i128()
.map_or_else(|| Err(Error::LiquidityAmountTooWide), Ok)?;
if giving {
// If we're giving, then we need to take from the delta.
delta = -delta;
}
#[cfg(feature = "testing-dbg")]
dbg!((
"inside adjust_position",
current_test!(),
sqrt_ratio_x_96.to_string(),
sqrt_ratio_a_x_96.to_string(),
sqrt_ratio_b_x_96.to_string(),
amount_0.to_string(),
amount_1.to_string(),
delta
));
// [update_position] should also ensure that we don't do this on a pool that's not currently
// running
self.update_position(id, delta)
}Math
While the submission is elaborate and goes into great detail, I do not believe that a tangible issue can manifest from calculating the square root price from the current tick as it should match the current price whenever this particular function is invoked. As such, I would advise a PoC to be introduced for this particular submission.
prapandey031 (warden) commented:
@0xsomeone - The issue is not a mere use of the price of
current_tickinstead of thecurrent_sqrt_price. Rather, this report describes how the Superposition protocol uses the wrong Uniswap v3 liquidity math formula.Uniswap v3 wrote a blog describing the relationship between
current_tick,current_tick_price, andcurrent_sqrt_price. The blog states that:This explains why using ticks can be less precise than
sqrtPriceX96in Uniswap v3.Just like ticks can be in between initialized ticks, ticks can also be in between integers as well! The protocol itself reports the floor
⌊ic⌋of the current tick, which thesqrtPriceX96retains.To simplify, getting the price of the
current_tickwould not be equal to getting thecurrent_sqrt_price, as Uniswap v3 stores thecurrent_tickas the floor of the tick derived through thecurrent_sqrt_price:Note: please see scenario in warden's original comment.
The above is equation 6.8 from Uniswap v3 whitepaper. The example in the blog makes it clear:
current_sqrt_price = 649004842.70137 price using current_tick = 648962487.5642413Note: please see scenario in warden's original comment.
Both parts lead to the wrong calculation of
Delta Liquidity. Finally, this results in the wrong calculation ofamount_0andamount_1whenadjust_position()function callsupdate_position(&mut self, id: U256, delta: i128)function with the wrongly calculateddelta:https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L299
self.update_position(id, delta)The root cause of the issue is the use of price of the
current_tickinstead of thecurrent_sqrt_price:https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L265
let sqrt_ratio_x_96 = tick_math::get_sqrt_ratio_at_tick(self.get_cur_tick().as_i32())?;This should be replaced with:
let sqrt_ratio_x_96 = self.sqrt_price.get();With this fix, the correct liquidity formulas are used for both parts. This issue is valid as the sponsor has mentioned faithful use of Uniswap v3 math as a core invariant in the Readme of the audit. This report successfully shows that this core invariant stands broken.
@prapandey031 - I greatly appreciate the in-depth mathematical analysis of the issue; however, a simple deviation of the mathematical formulae implemented by Uniswap V3 is not adequate to consider this an HM risk submission.
I would advise you to quantify the precise error that would result from this flooring mechanism of the tick, as well as produce a PoC that demonstrates the issue (i.e., an execution of the same operations in Uniswap V3 and the SuperPosition system) to illustrate the vulnerability in practice.
While the error is clear, we need to have a quantifiable impact to assess it as an HM risk.
prapandey031 (warden) commented:
@0xsomeone - I would quantify the funds that Superposition looses due to the explained error as well as detail other impacts.
Below I present calculation PoCs to quantify the impact. I break the complete PoC into 2 parts corresponding to the 2 parts in my previous response.
I build this response on top of my previous response. In my previous response, I proved that Superposition uses wrong liquidity equations. I did this using the in-scope code snippets of Superposition and Uniswapv3 math equations. In this response I calculate values based on the correct and wrong equations to quantify the impact.
Note: please see scenario in warden's original comment.
To confirm that the Uniswapv3 really uses the equations that I state above (although whatever equations stated above can be found in Uniswapv3 whitepaper; I took all of them from there) I provide the relevant code snippets from Uniswapv3-periphery repo:
addLiquidity(AddLiquidityParams memory params)which is similar toadjust_position()in Superposition:/// @notice Add liquidity to an initialized pool function addLiquidity(AddLiquidityParams memory params) internal returns ( uint128 liquidity, uint256 amount0, uint256 amount1, IUniswapV3Pool pool ) { PoolAddress.PoolKey memory poolKey = PoolAddress.PoolKey({token0: params.token0, token1: params.token1, fee: params.fee}); pool = IUniswapV3Pool(PoolAddress.computeAddress(factory, poolKey)); // compute the liquidity amount { > (uint160 sqrtPriceX96, , , , , , ) = pool.slot0(); uint160 sqrtRatioAX96 = TickMath.getSqrtRatioAtTick(params.tickLower); uint160 sqrtRatioBX96 = TickMath.getSqrtRatioAtTick(params.tickUpper); liquidity = LiquidityAmounts.getLiquidityForAmounts( sqrtPriceX96, sqrtRatioAX96, sqrtRatioBX96, params.amount0Desired, params.amount1Desired ); } (amount0, amount1) = pool.mint( params.recipient, params.tickLower, params.tickUpper, liquidity, abi.encode(MintCallbackData({poolKey: poolKey, payer: msg.sender})) ); require(amount0 >= params.amount0Min && amount1 >= params.amount1Min, 'Price slippage check'); } }Notice how Uniswapv3 uses
current_sqrt_priceinstead of price ofcurrent_tickat marked (>) LOC above. This is the fix that I proposed in the original report.
getLiquidityForAmounts()which is similar toget_liquidity_for_amounts()in Superposition:function getLiquidityForAmounts( uint160 sqrtRatioX96, uint160 sqrtRatioAX96, uint160 sqrtRatioBX96, uint256 amount0, uint256 amount1 ) internal pure returns (uint128 liquidity) { if (sqrtRatioAX96 > sqrtRatioBX96) (sqrtRatioAX96, sqrtRatioBX96) = (sqrtRatioBX96, sqrtRatioAX96); if (sqrtRatioX96 <= sqrtRatioAX96) { liquidity = getLiquidityForAmount0(sqrtRatioAX96, sqrtRatioBX96, amount0); } else if (sqrtRatioX96 < sqrtRatioBX96) { uint128 liquidity0 = getLiquidityForAmount0(sqrtRatioX96, sqrtRatioBX96, amount0); uint128 liquidity1 = getLiquidityForAmount1(sqrtRatioAX96, sqrtRatioX96, amount1); liquidity = liquidity0 < liquidity1 ? liquidity0 : liquidity1; } else { liquidity = getLiquidityForAmount1(sqrtRatioAX96, sqrtRatioBX96, amount1); } }
0xsomeone (judge) decreased severity to Medium and commented:
@prapandey031 - thank you for diving deeper into this particular submission! The original submission's impact indicated that transactions would fail when the minimum amounts are close to the expected amounts; however, your latest feedback indicates that actual fund loss would occur.
Given that the submission must be judged on its original merits and its validity has been demonstrated, I will award this as a medium-severity submission that would cause a portion of the protocol to become inaccessible versus resulting in fund loss, as this is an aspect not described in the original report.
af-afk (Superposition) confirmed
Submitted by zhaojohnson
LP holders will remove liquidity and then burn their liquidity position. If re-org happens, malicious users can manipulate the pool's price to cause user's removing liquidity revert. Then LP holders' position will be burned and liquidity will be locked in contract.
In lib.rs, users can burn their position. From this function's comment, Calling this function leaves any liquidity or fees left in the position inaccessible., it's user's responsibility to remove their liquidity before they burn their position.
The problem is that even if users follow this rule, remove liquidity at first, and then burn their position, users may still lose their funds.
Re-org is one common scenario in Arbitrum. When re-org happens, some transactions will be re-ordered and re-executed. For example:
- Alice removes her liquidity with one slippage control parameter.
- Alice burns her position.
- Re-org happens. Bob monitors the re-org and try to do some swap in pool to change pool's price and tick.
- Alice removing liquidity transaction will be executed and reverted because Bob changes current pool's price and Alice's slippage condition will not be matched.
- Alice's burning position can still be executed successfully.
- Alice will lose her funds.
pub fn burn_position_AE401070(&mut self, id: U256) -> Result<(), Revert> {
let owner = msg::sender();
// Get this NFT(position)'s owner to make sure that the caller owns this position.
assert_eq_or!(
self.position_owners.get(id),
owner,
Error::PositionOwnerOnly
);
self.remove_position(owner, id);
#[cfg(feature = "log-events")]
evm::log(events::BurnPosition { owner, id });
Ok(())
}Add some more check on burn_position_AE401070(), revert if there are some left liquidity.
Context
af-afk (Superposition) confirmed and commented:
We're going to resolve this by deleting the burn position feature.
The Warden has identified a mechanism in the Seawater system that can become harmful if a re-organization occurs, leading to a total loss of the funds associated with the position.
I believe a medium risk severity rating is appropriate given the likelihood of a re-organization simultaneously occurring with the sequence of events outlined (i.e., different blocks that ended up being re-ordered executing the actions outlined) is low.
Silvermist (warden) commented:
Arbitrum's docs states that a transaction can't be re-orged, so this should be invalid.
Yeah, there are no re-orgs in Arbitrum - https://abarbatei.xyz/blockchain-reorgs-for-managers-and-auditors#heading-metrics-for-protocols-and-auditors.
@Silvermist and @Blckhv - The documentation referenced states that if the L1 re-orgs, then the Arbitrum transactions will re-org as well. This is also referenced in the glossary itself.
The link referenced by @Blckhv also does not say that "re-orgs are not possible", simply that they have not been observed yet. As such, the original ruling of the submission stands.
Note: For full discussion, see here.
[M-05] When performing swap and the swap position does not cover swap amount, the base price of sqrt_price is set incorrectly
Submitted by 13u9
When performing a swap, a position is searched and if a position with an appropriate price exists, the swap is performed using the token amount of that position. If the positions in the current pool do not cover the swap amount requested by the user, the value of sqrt_price will not be set accurately, which may cause confusion when the next user swaps.
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L303-L535
When a swap is performed, the 'swap' function in pools.rs is triggered.
pub fn swap(
&mut self,
zero_for_one: bool,
amount: I256,
mut price_limit: U256,
) -> Result<(I256, I256, i32), Revert> {
assert_or!(self.enabled.get(), Error::PoolDisabled);
''' skip '''
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L378-L492
while !state.amount_remaining.is_zero() && state.price != price_limit {
iters += 1;
debug_assert!(iters < 500);
''' skip '''
When performing a swap, the while statement above iterates through the position that matches the tick and searches for it. If a matching position exists, the token of that position is processed and the state is updated.
The while statement will continue until the swap amount requested by the user becomes 0 or the price reaches the price_limit requested by the user.
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L496
self.sqrt_price.set(state.price);
After the while loop is finished, the final sqrt_price value is updated to the state.price value.
If the swap amount within the price_limit range requested by the user is not covered by the positions in the pool, the position price will be set to the price_limit value set by the user, not the position price that was actually swapped last; which will be different from the last swapped price, and the user requesting the next swap may be confused about the swap price.
Scenario:
- Assume that the following positions are currently formed in the pool:
pool1 state:
lower = 1000
upper = 2000
amount = 20
pool2 state:
lower = 3000
upper = 4000
amount = 20
-
Assume that the user sets
swap amountto100andprice_limitto10000and requests a swap. -
When the swap is in progress, it searches for a pool that can be swapped and processes
amount 20ofpool1andamount 30ofpool2to swap a total of 50 tokens. At this time, the position of the pool within theprice_limitset by the user cannot process theswap amountrequested by the user, so the while statement is performed untilstate.price == price_limit, and finally thesqrt_pricevalue is set to thestate.pricevalue. (state.price is equal to price_limit) -
After the swap routine is finished, the reference
swap pricewill be set to10000, which is theprice_limitset by the user, and not theswap priceofpool2where the last swap was done. -
Users requesting the next swap may experience confusion when processing the swap because the swap base price starts at the price of the
price_limit set by the previous user, rather than thebase priceof thepool where the last swap was made.
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L312-L331
match zero_for_one {
true => {
if price_limit == U256::MAX {
price_limit = tick_math::MIN_SQRT_RATIO + U256::one();
}
if price_limit >= self.sqrt_price.get() || price_limit <= tick_math::MIN_SQRT_RATIO
{
Err(Error::PriceLimitTooLow)?;
}
}
false => {
if price_limit == U256::MAX {
price_limit = tick_math::MAX_SQRT_RATIO - U256::one();
}
if price_limit <= self.sqrt_price.get() || price_limit >= tick_math::MAX_SQRT_RATIO
{
Err(Error::PriceLimitTooHigh)?;
}
}
};
Test Code POC is the code that sets zero_for_one to true and price_limit to U256::MAX and returns an error return in the above conditional statement when the second swap is performed.
Write Test Code in test/lib.rs file:
#[test]
fn test_poc1() -> Result<(), Vec<u8>> {
test_utils::with_storage::<_, Pools, _>(
Some(address!("feb6034fc7df27df18a3a6bad5fb94c0d3dcb6d5").into_array()),
None, // slots map
None, // caller erc20 balances
None, // amm erc20 balances
|contract| {
// Create the storage
contract.seawater_admin.set(msg::sender());
let token_addr = address!("97392C28f02AF38ac2aC41AF61297FA2b269C3DE");
// First, we set up the pool.
contract.create_pool_D650_E2_D0(
token_addr,
test_utils::encode_sqrt_price(100, 1), // the price
0,
1,
100000000000,
)?;
contract.enable_pool_579_D_A658(token_addr, true)?;
let lower_tick = 39122;
let upper_tick = 50108;
let liquidity_delta = 20000;
// create first position
contract.mint_position_B_C5_B086_D(token_addr, lower_tick, upper_tick)?;
let position_id = contract
.next_position_id
.clone()
.checked_sub(U256::one())
.unwrap();
// set position liquidity
contract.update_position_C_7_F_1_F_740(token_addr, position_id, liquidity_delta)?;
let (amount_out_0, amount_out_1) = contract.swap_904369_B_E(
token_addr,
true,
I256::try_from(100000_i32).unwrap(),
U256::MAX,
)?;
// At this time, the value of ```self.sqrt_price``` is ```tick_math::MIN_SQRT_RATIO```
// create second position
// Set lower_tick to the last swapped tick
let lower_tick = -887272;
let upper_tick = 887272;
let liquidity_delta = 20000000;
contract.mint_position_B_C5_B086_D(token_addr, lower_tick, upper_tick)?;
let id = U256::try_from(1).unwrap();
// set enough liquidity
contract.update_position_C_7_F_1_F_740(token_addr, id, liquidity_delta)?;
// try second swap
let (amount_out_0, amount_out_1) = contract.swap_904369_B_E(
token_addr,
true,
I256::try_from(100_i32).unwrap(),
U256::MAX,
)?;
Ok(())
},
)
}
Add println! code line and remove debugassert statement on swap function in src/pool.rs:
pub fn swap(
&mut self,
zero_for_one: bool,
amount: I256,
mut price_limit: U256,
) -> Result<(I256, I256, i32), Revert> {
assert_or!(self.enabled.get(), Error::PoolDisabled);
println!("self.sqrt_price: {}", self.sqrt_price.get()); // added
while !state.amount_remaining.is_zero() && state.price != price_limit {
iters += 1;
// debug_assert!(iters < 500);
Run Test:
cargo test test_poc1 --package seawater --features testing -- --nocapture
Result:
running 1 test
self.sqrt_price: 792281625142643375935439503360
self.sqrt_price: 4295128740
Error: [80, 114, 105, 99, 101, 32, 108, 105, 109, 105, 116, 32, 116, 111, 111, 32, 108, 111, 119]
test test_poc1 ... FAILED
failures:
failures:
test_poc1
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 16 filtered out; finished in 0.14s
error: test failed, to rerun pass `-p seawater --test lib`
When requesting a second swap, you can see that an error return occurs because sqrt_price is set to the price_limit set in the previous swap even though the position existed.
Visual Studio Code
Modify to allow setting the value to the price of the last swapped position even if the position amount is insufficient.
@@ -375,9 +375,10 @@ impl StoragePool {
// continue swapping while there's tokens left to swap
// and we haven't reached the price limit
let mut iters = 0;
+ let mut last_valid_price = state.price;
while !state.amount_remaining.is_zero() && state.price != price_limit {
iters += 1;
- debug_assert!(iters < 500);
+ // debug_assert!(iters < 500);
let step_initial_price = state.price;
@@ -479,6 +480,7 @@ impl StoragePool {
};
state.liquidity = liquidity_math::add_delta(state.liquidity, liquidity_net)?;
+ last_valid_price = state.price;
}
state.tick = match zero_for_one {
@@ -493,7 +495,8 @@ impl StoragePool {
// write state
// update price and tick
- self.sqrt_price.set(state.price);
+ self.sqrt_price.set(if state.price == price_limit { last_valid_price } else { state.price });
+
if state.tick != self.cur_tick.get().sys() {
self.cur_tick.set(I32::unchecked_from(state.tick));
}
Mitigation Test Result:
Test Code is same above Test Code
Running tests/lib.rs (target/debug/deps/lib-28fa4ebf2403ec3f)
running 1 test
self.sqrt_price: 792281625142643375935439503360
self.sqrt_price: 560222498985353939371108591955
test test_poc1 ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 16 filtered out; finished in 0.14s
Running tests/pools.rs (target/debug/deps/pools-a3343d45185ff606)
Unlike before, it is set to the tick where the last swap occurred.
Loop
af-afk (Superposition) confirmed
The Warden has correctly identified that the configuration of the square root price of the pool will be incorrect in case the price limit configured by the user has been achieved as the state's price is updated on each search iteration rather than being updated on the last valid swapped price.
I believe a medium-risk severity rating is appropriate given that a user can simply swap the price back down to the actual price of the AMM and no active harm may occur to the liquidity within the pool or its assets.
Submitted by ZanyBonzy, also found by shaflow2 and oakcobalt
decrPosition09293696 will not work, breaking SeawaterAMM.sol's functionality to decrease position.
SeaWaterAMM.sol holds the decrPosition09293696 which allows for refreshing a position's fees and takes liqiudity from it.
The function takes in various parameters but is missing the pool address parameter.
function decrPosition09293696(
uint256 /* id */,
uint256 /* amount0Min */,
uint256 /* amount1Min */,
uint256 /* amount0Max */,
uint256 /* amount1Max */
) external returns (uint256, uint256) {
directDelegate(_getExecutorUpdatePosition());
}As can be seen from the decr_position_09293696 function, the pool address must be declared from which liquidity can be taken from its positions.
pub fn decr_position_09293696(
&mut self,
pool: Address, //@audit
id: U256,
amount_0_min: U256,
amount_1_min: U256,
amount_0_max: U256,
amount_1_max: U256,
) -> Result<(U256, U256), Revert> {
//...
}
}Calls to the function will always fail, as a result, accessing the decrPosition09293696 function through SeawaterAMM.sol will always fail.
Introduce the pool address parameter:
function decrPosition09293696(
+ address /* pool */,
uint256 /* id */,
uint256 /* amount0Min */,
uint256 /* amount1Min */,
uint256 /* amount0Max */,
uint256 /* amount1Max */
) external returns (uint256, uint256) {
directDelegate(_getExecutorUpdatePosition());
}Context
af-afk (Superposition) confirmed
The Warden has correctly identified that the function definitions of the Solidity and Stylus contracts differ, resulting in the relevant functionality of the system being inaccessible.
I believe a medium-risk rating is appropriate for this vulnerability given that it is possible to interact with positions via the adjust position function and thus the functionality is simply inaccessible via the decrease position pathway rather than being inaccessible completely.
Submitted by DadeKuma
A possible wrong amount of the amount already swapped in/out of the input/output asset of a swap due to an unintended under/overflow.
In Rust release mode, the - and + operations will not revert when an underflow/overflow occurs.
As such, the amount can under/overflow in the while loop:
match exact_in {
true => {
state.amount_remaining -=
I256::unchecked_from(step_amount_in + step_fee_amount);
-> state.amount_calculated -= I256::unchecked_from(step_amount_out); //@audit this can underflow with -=
}
false => {
state.amount_remaining += I256::unchecked_from(step_amount_out);
-> state.amount_calculated += //@audit this can overflow with +=
I256::unchecked_from(step_amount_in + step_fee_amount);
}
}The issue is that these are not meant to under/overflow as seen in the original Uniswap logic, as they use SafeMath:
if (exactInput) {
state.amountSpecifiedRemaining -= (step.amountIn + step.feeAmount).toInt256();
-> state.amountCalculated = state.amountCalculated.sub(step.amountOut.toInt256());
} else {
state.amountSpecifiedRemaining += step.amountOut.toInt256();
-> state.amountCalculated = state.amountCalculated.add((step.amountIn + step.feeAmount).toInt256());
}Uniswap tests that state.amountSpecifiedRemaining > step.amountIn + step.feeAmount and that's why the operation is safe to use unchecked, but this is not the case with the second one, which must be checked:
https://github.com/Uniswap/v3-core/blob/0.8/contracts/UniswapV3Pool.sol#L681-L685
Consider using checked_sub instead of - (and checked_add instead of +):
match exact_in {
true => {
state.amount_remaining -=
I256::unchecked_from(step_amount_in + step_fee_amount);
- state.amount_calculated -= I256::unchecked_from(step_amount_out);
+ state.amount_calculated = state.amount_calculated.checked_sub(I256::unchecked_from(step_amount_out));
}
false => {
state.amount_remaining += I256::unchecked_from(step_amount_out);
- state.amount_calculated +=
- I256::unchecked_from(step_amount_in + step_fee_amount);
+ state.amount_calculated = state.amount_calculated.checked_add(
+ I256::unchecked_from(step_amount_in + step_fee_amount));
}
}Uniswap
af-afk (Superposition) confirmed
The submission details that the swap step calculations will perform an insecure subtraction or addition which might result in uncaught overflows/underflows occurring that should normally be prevented.
While I am inclined to retain a medium-risk severity rating for this submission for now, I would advise the Warden to substantiate their claim via a PoC that demonstrates the vulnerability in practice. If no PoC is provided, I will re-evaluate after PJQA and potentially downgrade to QA (L) due to insufficient proof.
Submitted by Silvermist, also found by zhaojohnson, nslavchev, Testerbot, devival, and prapandey031
There is a functionality for disabling a pool in case of unforeseen circumstances or if a problem occurs. While the pool is disabled the users should not be able to do certain actions but removing their liquidity should not be one of them.
When a problem occurs in the pool, users should be able to remove their liquidity, as it may be at risk and they may lose their money. The problem is there is a check that does not allow them to do it.
update_position_internal has a comment that states "Requires the pool to be enabled unless removing liquidity.", meaning that there should be a check for the pool status only when liquidity is added and the users should be allowed to remove their liquidity if the pool is not active.
But update_position_internal calls update_position that is used to add or remove liquidity from a position and the function has this check:
assert_or!(self.enabled.get(), Error::PoolDisabled);The check above prevents the function from being called when the pool is disabled, which is understandable in case users want to add liquidity to the pool when there is a problem.
However, users don't want their money to be at risk and they would want to remove their funds but they can't because of that check.
If the passed delta is negative, it means the user is removing liquidity, so check the pool's status only when the delta is positive.
Access Control
af-afk (Superposition) confirmed
The submission and its duplicates have correctly identified that liquidity withdrawals are disallowed when the pool is paused despite what the code's documentation indicates.
I believe a medium-risk severity rating is appropriate given that important functionality of the protocol is inaccessible in an emergency scenario when it should be accessible.
[M-09] swap_2 implementation will randomly revert due to improper check, root cause for failed test ethers_suite_uniswap_orchestrated_uniswap_two
Submitted by oakcobalt, also found by pipidu83, prapandey031, and Silvermist
swap_2 implementation will randomly revert due to improper check, root cause for failed test ethers_suite_uniswap_orchestrated_uniswap_two.
The audit readme listed an unsolved failed test ethers_suite_uniswap_orchestrated_uniswap_two(). The failed test is due to an improper check in swap_2_internal(), which sometimes causes a valid swap_2 to revert.
We see that in swap_2_internal, interim_usdc_out is required to equal interim_usdc_in. interim_usdc_out is the fusdc swapped out in 1st swap. interim_usdc_in is the actual fusdc swapped in 2nd swap. In any swap, not all user input amount has to be used to achieve a desirable output. Especially in uniswapV3 swap logic, amount_remaining can be greater than 0, which means not all input tokens are used.
//pkg/seawater/src/lib.rs
fn swap_2_internal(
pools: &mut Pools,
from: Address,
to: Address,
amount: U256,
min_out: U256,
) -> Result<(U256, U256, U256, I256, i32, i32), Revert> {
...
// swap in -> usdc
let (amount_in, interim_usdc_out, final_tick_in) = pools.pools.setter(from).swap(
true,
amount,
// swap with no price limit, since we use min_out instead
tick_math::MIN_SQRT_RATIO + U256::one(),
)?;
...
// swap usdc -> out
let (amount_out, interim_usdc_in, final_tick_out) = pools.pools.setter(to).swap(
false,
interim_usdc_out,
tick_math::MAX_SQRT_RATIO - U256::one(),
)?;
...
|> assert_eq_or!(interim_usdc_out, interim_usdc_in, Error::InterimSwapNotEq);
...The equality check will cause revert when 2nd swap runs out of liquidity but still satisfies the user's min_out. A valid swap can be reverted.
Test ethers_suite_uniswap_orchestrated_uniswap_two() analysis:
When running the test, the output error byte array [73, 110, 116, 101, 114, 110, 97, 108, 32, 115, 119, 97, 112, 32, 97, 109, 111, 117, 110, 116, 115, 32, 110, 111, 116, 32, 109, 97, 116, 99, 104, 101, 100] decodes(ASII) into Internal swap amounts not matched, corresponding to Error::InterimSwapNotEq. This custom error is only used in swap_2_internal.
thread 'ethers_suite_orchestrated_uniswap_two' panicked at seawater/tests/lib.rs:783:18:
called `Result::unwrap()` on an `Err` value: [73, 110, 116, 101, 114, 110, 97, 108, 32, 115, 119, 97, 112, 32, 97, 109, 111, 117, 110, 116, 115, 32, 110, 111, 116, 32, 109, 97, 116, 99, 104, 101, 100]
I added a unit test to break down the failed reason ethers_suite_orchestrated_uniswap_two_breakdown:
//pkg/seawater/tests/lib.rs
#[test]
fn ethers_suite_orchestrated_uniswap_two_breakdown() {
test_utils::with_storage::<_, Pools, _>(
Some(address!("3f1Eae7D46d88F08fc2F8ed27FCb2AB183EB2d0E").into_array()), // sender
None,
None,
None,
|contract| {
let token0 = address!("9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0");
let token1 = address!("9fE46736679d2D9a65F0992F2272dE9f3c7fa6e1");
contract
.ctor(msg::sender(), Address::ZERO, Address::ZERO)
.unwrap();
contract
.create_pool_D650_E2_D0(
token0,
U256::from_limbs([0, 42949672960, 0, 0]), //792281625142643375935439503360
500, // fee
10, // tick spacing
u128::MAX,
)
.unwrap();
contract
.create_pool_D650_E2_D0(
token1,
U256::from_limbs([0, 42949672960, 0, 0]), //792281625142643375935439503360
500, // fee
10, // tick spacing
u128::MAX,
)
.unwrap();
contract.enable_pool_579_D_A658(token0, true).unwrap();
contract.enable_pool_579_D_A658(token1, true).unwrap();
contract
.mint_position_B_C5_B086_D(token0, 39120, 50100)
.unwrap();
contract
.mint_position_B_C5_B086_D(token1, 39120, 50100)
.unwrap();
let id = U256::ZERO;
let (amount_0_in, fusdc_0_in) = contract
.update_position_C_7_F_1_F_740(token0, id, 20000)
.unwrap();
println!("amount_0_in: {amount_0_in}, fusdc_0_in: {fusdc_0_in}");
let (amount_1_in, fusdc_1_in) = contract
.update_position_C_7_F_1_F_740(token1, U256::one(), 20000)
.unwrap();
println!("amount_1_in: {amount_1_in}, fusdc_1_in: {fusdc_1_in}");
//breakdown: step1 token0 -> fusdc
let (amount_out_0, fusdc_out_0) = contract
.swap_904369_B_E(token0, true, I256::try_from(1000_i32).unwrap(), U256::MAX)
.unwrap();
println!("amount_out_0: {amount_out_0}, fusdc_out_0: {fusdc_out_0}");
//breakdown: step2 fusdc -> token1
let (amount_out_1, fusdc_out_1) = contract
.swap_904369_B_E(token1, false, -fusdc_out_0, U256::MAX)
.unwrap();
println!("amount_out_1: {amount_out_1}, fusdc_out_1: {fusdc_out_1}");
assert_ne!(-fusdc_out_0, fusdc_out_1);
},
);
}Test result:
Running tests/lib.rs (target/debug/deps/lib-b27e97df8f1d4fcd)
running 1 test
amount_0_in: 367, fusdc_0_in: 58595
amount_1_in: 367, fusdc_1_in: 58595
amount_out_0: 833, fusdc_out_0: -58592
amount_out_1: -365, fusdc_out_1: 44866
test ethers_suite_orchestrated_uniswap_two_breakdown ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 16 filtered out; finished in 0.04s
As seen above, due to in the 2nd swap (fusdc → token1), the token1 pool ran out of liquidity, only a portion of fusdc_out_0 is used to perform the swap. fusdc_out_0 ≠ fusdc_out_1 reverted the original ethers_suite_uniswap_orchestrated_uniswap_two test.
VScode
Consider removing/relaxing the strict equality check.
af-afk (Superposition) confirmed
The submission and its duplicates have correctly identified that a restriction imposed in
lib::swap_2_internalwill cause proper swaps to fail due to mandating strict equality for the funds consumed during a swap.I believe a medium risk rating is appropriate given that it will cause a subset of transactions to fail with no harm beyond a Denial-of-Service.
Submitted by shaflow2, also found by ZanyBonzy, OMEN, nnez, and oakcobalt
In the swap_2_internal function, if the first pool experiences a liquidity depletion, it could result in amount_in being less than original_amount. However, the contract might still require the user to transfer original_amount of tokens, causing the user to pay more tokens.
When calling swap2 to convert token1 to token2 through fusdc, if the token1-fusdc pool experiences a liquidity depletion, it will lead to the issues that the calculated amount_in of token1 will be less than the expected input original_amount.
fn swap_2_internal(
pools: &mut Pools,
from: Address,
to: Address,
amount: U256,
min_out: U256,
) -> Result<(U256, U256, U256, I256, i32, i32), Revert> {
let original_amount = amount;
let amount = I256::try_from(amount).map_err(|_| Error::SwapResultTooHigh)?;
// swap in -> usdc
let (amount_in, interim_usdc_out, final_tick_in) = pools.pools.setter(from).swap(
true,
amount,
// swap with no price limit, since we use min_out instead
tick_math::MIN_SQRT_RATIO + U256::one(),
)?;
...
}After the swap_2_internal function completes, if the user is required to pay the protocol original_amount of token1, they end up paying original_amount - amount_in more tokens than they actually receive.
pub fn swap_2_internal_erc20(
pools: &mut Pools,
from: Address,
to: Address,
amount: U256,
min_out: U256,
permit2: Option<Permit2Args>,
) -> Result<(U256, U256), Revert> {
let (
original_amount,
amount_in,
amount_out,
_interim_usdc_out,
_final_tick_in,
_final_tick_out,
) = Self::swap_2_internal(pools, from, to, amount, min_out)?;
// transfer tokens
erc20::take(from, original_amount, permit2)?;
erc20::transfer_to_sender(to, amount_out)?;
Ok((amount_in, amount_out))
}
}Since compatibility with Permit2 is required, it is recommended to refund the excess tokens back to the user.
pub fn swap_2_internal_erc20(
pools: &mut Pools,
from: Address,
to: Address,
amount: U256,
min_out: U256,
permit2: Option<Permit2Args>,
) -> Result<(U256, U256), Revert> {
let (
original_amount,
amount_in,
amount_out,
_interim_usdc_out,
_final_tick_in,
_final_tick_out,
) = Self::swap_2_internal(pools, from, to, amount, min_out)?;
// transfer tokens
erc20::take(from, original_amount, permit2)?;
erc20::transfer_to_sender(to, amount_out)?;
+ if(original_amount > amount_in){
+ erc20::transfer_to_sender(to, original_amount - amount_in)?;
+ }
Ok((amount_in, amount_out))
}
}af-afk (Superposition) confirmed and commented:
See commit.
The submission and its duplicates have correctly identified that a
swap_2_internal_erc20operation will result in an overpayment of funds if the input amount is not consumed in full.I believe a medium risk severity rating to be appropriate for this submission as it can lead to a minor fund loss under certain circumstances for the user.
Submitted by wasm_it
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L53
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L67
Provision of liquidity for pools with high volatility will not be possible as such pools cannot be initiated with a tick spacing basis point greater than 256 because of the tick_spacing datatype constrained to a u8.
Less volatile pools usually have fee tiers in the range of 0.05% to 1%. But higher volatile pools go over 1% as a baseline and some pools can have a 3-5% fee tier depending on the volatility of the pool pairs. Since the protocol intends to be able to create pools for any tokens with sufficient liquidity, pools with exotic pairs in the fee range of > 1.3% cannot be instantiated.
pub fn init(
&mut self,
price: U256,
fee: u32,
@> tick_spacing: u8, // e.g 300 for a pool with 1.5% fee tier
max_liquidity_per_tick: u128,
) -> Result<(), Revert> {
assert_eq_or!(
self.sqrt_price.get(),
U256::ZERO,
Error::PoolAlreadyInitialised
);
self.sqrt_price.set(price);
self.cur_tick
.set(I32::lib(&tick_math::get_tick_at_sqrt_ratio(price)?));
self.fee.set(U32::lib(&fee));
@> self.tick_spacing.set(U8::lib(&tick_spacing)); // @audit would revert at this point because the value exceeds the size of the datatype
self.max_liquidity_per_tick
.set(U128::lib(&max_liquidity_per_tick));
Ok(())
}Uniswap uses a bigger datatype for this such as a 24-bit integer type. A bigger datatype would be sufficient for this such as u16.
Context
af-afk (Superposition) confirmed and commented:
See commit.
We wound up enforcing a limit on the fee after some discussion that we don't intend to set fees that high.
The submission has correctly identified that the data type used for the tick spacing of AMM pools is insufficient for volatile pools, causing them to become unusable due to their prices moving across ticks too swiftly.
I believe a medium-risk severity rating is appropriate given that the current AMM model is curated for volatile rather than stable pairs, rendering this scenario to have a non-negligible likelihood of manifesting in practice.
Submitted by shaflow2, also found by prapandey031, zhaojohnson, DadeKuma, peanuts, oakcobalt, and wasm_it
There are no related functions to set fee_protocol, which prevents the protocol from accumulating protocol fees.
The pool contract defines the protocol fee rate fee_protocol, but there is no function to set it.
#[solidity_storage]
pub struct StoragePool {
...
fee_protocol: StorageU8,
fee_growth_global_0: StorageU256,
fee_growth_global_1: StorageU256,
protocol_fee_0: StorageU128,
protocol_fee_1: StorageU128,
...
}The contract also defines the collect_protocol and collect_protocol_7540_F_A_9_F functions to collect fees. However, since the protocol fee rate cannot be set, the protocol will never accumulate any protocol fees.
#[allow(non_snake_case)]
pub fn collect_protocol_7540_F_A_9_F(
&mut self,
pool: Address,
amount_0: u128,
amount_1: u128,
recipient: Address,
) -> Result<(u128, u128), Revert> {
...
} pub fn collect_protocol(
&mut self,
amount_0: u128,
amount_1: u128,
) -> Result<(u128, u128), Revert> {
...
}Add the relevant functions to enable protocol fees.
pkg/seawater/src/pool.rs:
+ pub fn set_fee_protocol(&mut self, new_fee_protocol: U256) {
+ self.fee_protocol.set(new_fee_protocol);
+ }pkg/seawater/src/lib.rs:
+ #[allow(non_snake_case)]
+ pub fn set_fee_protocol(
+ &mut self,
+ pool: Address,
+ new_fee_protocol: U256,
+ ) -> Result<(), Revert> {
+ assert_eq_or!(
+ msg::sender(),
+ self.seawater_admin.get(),
+ Error::SeawaterAdminOnly
+ );
+
+ self.pools.setter(pool).set_fee_protocol(new_fee_protocol);
+
+ Ok(())
+ }af-afk (Superposition) confirmed and commented:
See commit.
The submission and its duplicates have correctly identified that there is no mechanism to set the protocol fee in the system, causing fees to never accumulate in the current implementation.
I believe a severity of medium is appropriate given that its only harm is prospective earnings for the protocol itself.
For this audit, 16 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by ZanyBonzy received the top score from the judge.
The following wardens also submitted reports: oakcobalt, DadeKuma, prapandey031, SBSecurity, Tigerfrake, adeolu, ABAIKUNANBAEV, zhaojie, swapnaliss, devival, zhaojohnson, Testerbot, Nikki, shaflow2, and Silvermist.
tokenURI doesn't enforce that the token being checked is a valid token, which can lead to malicious users impersonating bogus NFTs as the ownership NFT. Unsuspecting users would attempt to query the token uri and would be returned with a valid uri, since the function doesn't ensure that the tokenId is a valid NFT.
Also, according to EIP-721 metadata standard, the function should revert if _tokenId is not a valid NFT.
/// @notice A distinct Uniform Resource Identifier (URI) for a given asset. /// @dev Throws if
_tokenIdis not a valid NFT. URIs are defined in RFC
function tokenURI(uint256 /* _tokenId */) external view returns (string memory) {
return TOKEN_URI;
}Add the check for NFT existence in the function by checking that the owner is not address(0).
The approve function allows operator and owner to approve themselves to spend the token. This can be used to give themselves a backdoor to the token. If for example the operator's status is to be revoked, he can always call the approve function on the token, to approve himself to spend the token. So even if his operator status is revoked, he still has access to the tokens.
function approve(address _approved, uint256 _tokenId) external payable {
_requireAuthorised(msg.sender, _tokenId);
getApproved[_tokenId] = _approved;
}Ensure that the owner/operator cannot approve themselves to spend a token.
transfer_position_E_E_C7_A3_C_D directly removes the positions from the sender and adds it to the receiver without making considerations for potential fees that the position has accrued.
pub fn transfer_position_E_E_C7_A3_C_D(
&mut self,
id: U256,
from: Address,
to: Address,
) -> Result<(), Revert> {
assert_eq_or!(msg::sender(), self.nft_manager.get(), Error::NftManagerOnly);
self.remove_position(from, id);
self.grant_position(to, id);
#[cfg(feature = "log-events")]
evm::log(events::TransferPosition { from, to, id });
Ok(())
}Recommend introducing collect_single_to_6_D_76575_F into the transfer_position_E_E_C7_A3_C_D function to help users directly claim fees before transferring their positions.
The collect and collect_protocol functions allow users and protocol admins to claim their fees. These functions are, however, not accessible when the pool is disabled. As a rule of thumb, functions to remove rewards or withdraw tokens from a protocol should always be accessible to help users claim resuce their tokens. Also a malicious admin can disable the pool and renounce his ownership, permanently locking the tokens in the protocol.
pub fn collect(&mut self, id: U256) -> Result<(u128, u128), Revert> {
>> assert_or!(self.enabled.get(), Error::PoolDisabled);
Ok(self.positions.collect_fees(id))
} pub fn collect_protocol(
&mut self,
amount_0: u128,
amount_1: u128,
) -> Result<(u128, u128), Revert> {
>> assert_or!(self.enabled.get(), Error::PoolDisabled);
let owed_0 = self.protocol_fee_0.get().sys();
let owed_1 = self.protocol_fee_1.get().sys();
let amount_0 = u128::min(amount_0, owed_0);
let amount_1 = u128::min(amount_1, owed_1);
if amount_0 > 0 {
self.protocol_fee_0.set(U128::lib(&(owed_0 - amount_0)));
}
if amount_1 > 0 {
self.protocol_fee_1.set(U128::lib(&(owed_1 - amount_1)));
}
Ok((amount_0, amount_1))
}Recommend removing the check for pool activity in the functions.
The incrPositionPermit25468326E function is implemented in SeawaterAMM.sol, but isn't implemented in any of the pool's backend library. As a result any calls to the function will always fail.
function incrPositionPermit25468326E(
address /* token */,
uint256 /* id */,
uint256 /* amount0Min */,
uint256 /* amount1Min */,
uint256 /* nonce0 */,
uint256 /* deadline0 */,
uint256 /* amount0Max */,
bytes memory /* sig0 */,
uint256 /* nonce1 */,
uint256 /* deadline1 */,
uint256 /* amount1Max */,
bytes memory /* sig1 */
) external returns (uint256, uint256) {
directDelegate(_getExecutorUpdatePosition());
}Recommend removing the unimplemented function.
A number of functions in OwnershipNFTs.sol are marked as payable, meaning they can receive ETH. But the contract implementation doesn't make use of ETH, nor is there any function to recover any ETH sent. As a result the tokens will be lost forever.
function transferFrom(
address _from,
address _to,
uint256 _tokenId,
bytes calldata /* _data */
) external payable {
// checks that the user is authorised
_transfer(_from, _to, _tokenId);
}
/// @inheritdoc IERC721Metadata
function transferFrom(
address _from,
address _to,
uint256 _tokenId
) external payable {
// checks that the user is authorised
_transfer(_from, _to, _tokenId);
}
/// @inheritdoc IERC721Metadata
function safeTransferFrom(
address _from,
address _to,
uint256 _tokenId
) external payable {
_transfer(_from, _to, _tokenId);
_onTransferReceived(msg.sender, _from, _to, _tokenId);
}
/// @inheritdoc IERC721Metadata
function safeTransferFrom(
address _from,
address _to,
uint256 _tokenId,
bytes calldata /* _data */
) external payable {
_transfer(_from, _to, _tokenId);
_onTransferReceived(msg.sender, _from, _to, _tokenId);
}
/// @inheritdoc IERC721Metadata
function approve(address _approved, uint256 _tokenId) external payable {
_requireAuthorised(msg.sender, _tokenId);
getApproved[_tokenId] = _approved;
}Recommend removing the payable modifier from the listed functions, or introducing a function to recover any excess tokens in the contract.
lib.rs holds the adjust_position_internal which allows users to change their position details according to specified giving bool. However, this function is not exposed in SeawaterAMM.sol, so as such cannot be directly queried. Also, since there's an issue with the decrPosition09293696 function (an incorrect function definition), there's no way to for users to reduce their positions.
Recommend exposing the function.
The swap_permit_2_E_E84_A_D91, swap_2_exact_in_permit_2_36_B2_F_D_D8, incrPositionPermit25468326E, swapOutPermit23273373B, swapOut5E08A399, swapInPermit2CEAAB576, swap2ExactInPermit236B2FDD8 etc., functions allow off-chain signed approvals to be used on-chain, saving gas and improving user experience.
However, including these functions will make the transaction susceptible to DOS via frontrunning. An attacker can observe the transaction in the mempool, extract the permit signature and values from the calldata and execute the permit before the original transaction is processed. This would consume the nonce associated with the user's permit and cause the original transaction to fail due to the now-invalid nonce.
This attack vector has been previously described in Permission Denied - The Story of an EIP that Sinned.
Recommend wrapping the permit functionalities in a try catch block or its rust equivalent. Something like this may work.
try IERC20Permit(token).permit(msgSender, address(this), value, deadline, v, r, s) {
// Permit executed successfully, proceed
} catch {
// Check allowance to see if permit was already executed
uint256 currentAllowance = IERC20(token).allowance(msgSender, address(this));
if(currentAllowance < value) {
revert("Permit failed and allowance insufficient");
}
// Otherwise, proceed as if permit was successful
}Functions swap_904369_B_E, swap_2_exact_in_41203_F1_D, swap_permit_2_E_E84_A_D91, swap_2_exact_in_permit_2_36_B2_F_D_D8, swapOutPermit23273373B, swapOut5E08A399, swapIn32502CA71, swapInPermit2CEAAB576 etc., are missing deadline checks.
As front-running is a key aspect of AMM design, the deadline is a useful tool to ensure that user transactions cannot be "saved for later" by miners or stay longer than needed in the mempool. The longer transactions stay in the mempool, the more likely it is that MEV bots can steal positive slippage from the transaction.
Consider introducing a deadline parameter in all the pointed-out functions.
Not sure the best severity for this, as the contract is described as an interface for tracking ownership positions. Yet at the same time, it holds functionalities that are pertinent to real NFTs, it can be approved and transfered. In any case, the contract lacks any functionality to mint and burn the tokens. As such, these tokens can't really be minted or burnt from users.
Also, when positions are minted/burned from the pool, no function is called to mint/burn NFTs from the caller.
pub fn mint_position_B_C5_B086_D(
&mut self,
pool: Address,
lower: i32,
upper: i32,
) -> Result<U256, Revert> {
let id = self.next_position_id.get();
self.pools.setter(pool).create_position(id, lower, upper)?;
self.next_position_id.set(id + U256::one());
let owner = msg::sender();
self.grant_position(owner, id);
#[cfg(feature = "log-events")]
evm::log(events::MintPosition {
id,
owner,
pool,
lower,
upper,
});
Ok(id)
} #[allow(non_snake_case)]
pub fn burn_position_AE401070(&mut self, id: U256) -> Result<(), Revert> {
let owner = msg::sender();
assert_eq_or!(
self.position_owners.get(id),
owner,
Error::PositionOwnerOnly
);
self.remove_position(owner, id);
#[cfg(feature = "log-events")]
evm::log(events::BurnPosition { owner, id });
Ok(())
}Lots of the contracts in scope still hold the debugging code. As a result, the codebase looked a bit cluttered. Also, the test suite and its configuration was very difficult to get under control. Lots of weird errors and failures.
#[cfg(feature = "testing-dbg")]transferFrom and safeTransferFrom take the calldata parameter but its not in used, especially in the _onTransferReceived hook. External integrations depending on the calldata will have issues due to the parameter not being used.
function transferFrom(
address _from,
address _to,
uint256 _tokenId,
bytes calldata /* _data */
) external payable {
// checks that the user is authorised
_transfer(_from, _to, _tokenId);
} function safeTransferFrom(
address _from,
address _to,
uint256 _tokenId,
bytes calldata /* _data */
) external payable {
_transfer(_from, _to, _tokenId);
_onTransferReceived(msg.sender, _from, _to, _tokenId);
}Recommend implementing the use of this parameter, or removing the functions since it's redundant.
transfer_position_E_E_C7_A3_C_D allows users to transfer tokens to themselves. It should return if from == to.
pub fn transfer_position_E_E_C7_A3_C_D(
&mut self,
id: U256,
from: Address,
to: Address,
) -> Result<(), Revert> {
assert_eq_or!(msg::sender(), self.nft_manager.get(), Error::NftManagerOnly);
self.remove_position(from, id);
self.grant_position(to, id);
#[cfg(feature = "log-events")]
evm::log(events::TransferPosition { from, to, id });
Ok(())
}swap_2_exact_in_41203_F1_D allows swapping between the same pools which malicious users can attempt to leverage, trying to drain pool liqudity. Recommend reverting or returning if from == to.
pub fn swap_2_exact_in_41203_F1_D(
&mut self,
from: Address,
to: Address,
amount: U256,
min_out: U256,
) -> Result<(U256, U256), Revert> {
Pools::swap_2_internal_erc20(self, from, to, amount, min_out, None)
}
}af-afk (Superposition) commented:
[L-01] - This wasn't a goal, but we'll do fix it anyway.
[L-02] - We'll allow this, in some circumstances (hack of a third party), we want to have the power to do this. We'll delegate the power to a DAO.
[L-03] - We won't implement this. It should be the responsibility of the caller to know when to collect accrued fees.
[L-04] - We won't fix this, the single contract environment increases the risk profile of the math going wrong, so we don't want to support users collecting fees in an emergency context. Also, the admin being malicious is not a part of the threat model.
[L-05] - We'll remove this.
[L-06] - We'll remove the payable functions.
[L-07] - This will be fixed with correctdecrPositioncode. We won't make the recommendations in this submission.
[L-08] - That's very interesting! We won't however fix it, we don't believe that in practice this is likely to take place.
[L-09] - We have a centralised sequencer, the AMM is deployed on our chain. For this reason, we don't check for the deadline. So we won't implement a change for this. Also, the end user is ultimately going to be taking advantage ofpermit2, which includes a deadline anyway.
[L-10] - We won't make these suggestions! It should mirror the main contract and we won't include the NFT manager in the UX flow for positions.
[L-11] - We acknowledge it's cluttered in its current state, and we'll make an adjustment in the future to improve the error reporting for debugging. But we won't immediately make these fixes.
[L-12] - We won't make this change.
[L-13] - We won't make this change, if the user wants to waste their time without any harmful side effects with something like this, they're welcome to.
[L-14] - We'll make this change.
C4 is an open organization governed by participants in the community.
C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity and rust developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.