-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spearbit post-audit fixes #89
Changes from all commits
95b6928
8cff3fc
8c675cc
0557365
27381ed
8040789
d30b822
43b769a
e1c4eea
849cac0
6594c38
c62ca0e
fe89db1
bf1102c
ed37a9b
1fe6809
6d3eeb9
051f786
cfd7ddc
163d7d3
21031b5
fa84ff1
88b596b
982765a
746cbc1
3f2bd90
f3698b1
5fcbba3
94a304d
bd3c680
b30f8b4
f82f84c
4f29be9
9f6da08
c63ce0c
8fbbc8c
a946a7c
1fe5af1
fb90559
8d8f8cb
0903141
af73745
ca780dc
3bd1bc3
6154ecd
63ed521
8ab9767
fba3699
687a0e3
dbdbe65
923b3a7
674f6aa
c9e4d9f
09fc5ec
436d805
8d6fffe
45e6364
55abf6c
f8a507a
1f70427
19b012e
7e930dc
43da6f2
22967e4
c2fdd68
e3f07d2
03bf4f4
7033738
df79f5a
e6496e6
5242573
c6fa69b
c2f0b5f
6e84fcf
3e9497c
de6d27b
8aebaae
d18948c
5a37380
99b0158
ad69860
3f94e01
dad2ff5
869f482
bda4065
e591dea
28b310c
c9c6f53
b707a67
e05bcb8
4d4b108
8c8ce08
9562053
74912bc
6a3c914
a6a9ae3
2bec4ff
166cbaf
a984a6d
ba12fb2
85a1695
fc63b45
9bc37f2
fdadbbd
46c1027
7699e52
6f12c05
8fc862a
4052863
f182bac
1a2a4b4
95744d2
e70b54e
740d28d
dce5f77
eccb50c
78ddd8c
a8f02f3
6311ddc
e7a7867
1317937
744e716
da8d520
802491b
5c52003
28315ce
a7cf0aa
26f2f8c
0f5fc6f
80bc05e
d5b485d
d4b6fd2
6d2faab
9605b1f
3c3b1ae
7b8df07
6001c4e
276ceb3
3333eaf
e5db5ac
5a7b2bc
6ecc8e7
d9f23f8
c6209b8
86b6b0b
af6ac26
62eeb36
fa3af60
c38bb61
8cb531c
274696a
22313f9
7406c0e
49a20c7
0f0ce53
2069ef1
8a6788f
8a7fa28
03ea755
11861e8
1316bc2
74719d5
f332e3d
a6ed4ed
cda904a
cebcdca
8bff684
1972c1c
aa9e127
32a5572
f597df3
79c7b22
4fd811a
bf3c7bf
1b9e6ed
a2f111e
b0c9f6f
1b7a4dd
7868687
af5c660
89e1a96
32f9862
1a3d7b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,18 @@ | |
pragma solidity 0.8.22; | ||
|
||
import { | ||
FixedPointMathLib, computeTradingFunction | ||
FixedPointMathLib, | ||
computeTradingFunction, | ||
computeSwapDeltaLiquidity, | ||
computeDeltaLiquidity | ||
} from "./ConstantSumMath.sol"; | ||
import { | ||
decodePriceUpdate, | ||
decodeFeeUpdate, | ||
decodeControllerUpdate | ||
} from "./ConstantSumUtils.sol"; | ||
import { PairStrategy, IStrategy, Pool } from "src/PairStrategy.sol"; | ||
import { EPSILON } from "src/lib/StrategyLib.sol"; | ||
|
||
struct InternalParams { | ||
uint256 price; | ||
|
@@ -33,6 +37,9 @@ enum UpdateCode { | |
contract ConstantSum is PairStrategy { | ||
using FixedPointMathLib for uint256; | ||
|
||
/// @notice Thrown when the expected liquidity is not met. | ||
error InvalidDeltaLiquidity(); | ||
|
||
/// @inheritdoc IStrategy | ||
string public constant name = "ConstantSum"; | ||
|
||
|
@@ -45,7 +52,7 @@ contract ConstantSum is PairStrategy { | |
function init( | ||
address, | ||
uint256 poolId, | ||
Pool calldata, | ||
Pool calldata pool, | ||
bytes calldata data | ||
) | ||
public | ||
|
@@ -58,21 +65,106 @@ contract ConstantSum is PairStrategy { | |
) | ||
{ | ||
ConstantSumParams memory params; | ||
(reserves, totalLiquidity, params) = | ||
abi.decode(data, (uint256[], uint256, ConstantSumParams)); | ||
|
||
(reserves, params) = abi.decode(data, (uint256[], ConstantSumParams)); | ||
totalLiquidity = | ||
computeDeltaLiquidity(reserves[0], reserves[1], params.price); | ||
|
||
if (pool.reserves.length != 2 || reserves.length != 2) { | ||
revert InvalidReservesLength(); | ||
} | ||
|
||
internalParams[poolId].price = params.price; | ||
internalParams[poolId].swapFee = params.swapFee; | ||
internalParams[poolId].controller = params.controller; | ||
|
||
// Get the trading function and check this is valid | ||
invariant = | ||
tradingFunction(reserves, totalLiquidity, abi.encode(params)); | ||
|
||
valid = invariant >= 0; | ||
valid = invariant >= 0 && invariant <= EPSILON; | ||
|
||
return (valid, invariant, reserves, totalLiquidity); | ||
} | ||
|
||
function validateAllocate( | ||
address, | ||
uint256 poolId, | ||
Pool memory pool, | ||
bytes calldata data | ||
) | ||
external | ||
view | ||
override | ||
returns ( | ||
bool valid, | ||
int256 invariant, | ||
uint256[] memory deltas, | ||
uint256 deltaLiquidity | ||
) | ||
{ | ||
(uint256 deltaX, uint256 deltaY, uint256 minDeltaL) = | ||
abi.decode(data, (uint256, uint256, uint256)); | ||
|
||
deltaLiquidity = | ||
computeDeltaLiquidity(deltaX, deltaY, internalParams[poolId].price); | ||
if (deltaLiquidity < minDeltaL) revert InvalidDeltaLiquidity(); | ||
Comment on lines
+106
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason why The parameters for This could be confusing to users - they might call this function thinking that the parameters are the same as the other strategies. I recommend documenting this difference so that users are aware. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, so other strategies are computing the deltas given a specific amount of liquidity delta but with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, in that case I don't think this inconsistency is avoidable. |
||
|
||
deltas = new uint256[](2); | ||
deltas[0] = deltaX; | ||
deltas[1] = deltaY; | ||
|
||
pool.reserves[0] += deltaX; | ||
pool.reserves[1] += deltaY; | ||
|
||
invariant = tradingFunction( | ||
pool.reserves, | ||
pool.totalLiquidity + deltaLiquidity, | ||
getPoolParams(poolId) | ||
); | ||
|
||
valid = invariant >= 0; | ||
} | ||
|
||
function validateDeallocate( | ||
address, | ||
uint256 poolId, | ||
Pool memory pool, | ||
bytes calldata data | ||
) | ||
external | ||
view | ||
override | ||
returns ( | ||
bool valid, | ||
int256 invariant, | ||
uint256[] memory deltas, | ||
uint256 deltaLiquidity | ||
) | ||
{ | ||
(uint256 deltaX, uint256 deltaY, uint256 maxDeltaL) = | ||
abi.decode(data, (uint256, uint256, uint256)); | ||
|
||
deltaLiquidity = | ||
computeDeltaLiquidity(deltaX, deltaY, internalParams[poolId].price); | ||
if (maxDeltaL > deltaLiquidity) revert InvalidDeltaLiquidity(); | ||
Comment on lines
+148
to
+150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is reversed, the function should be ensuring that - if (maxDeltaL > deltaLiquidity) revert InvalidDeltaLiquidity();
+ if (maxDeltaL < deltaLiquidity) revert InvalidDeltaLiquidity(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed here 13aa509. |
||
|
||
deltas = new uint256[](2); | ||
deltas[0] = deltaX; | ||
deltas[1] = deltaY; | ||
|
||
pool.reserves[0] -= deltaX; | ||
pool.reserves[1] -= deltaY; | ||
|
||
invariant = tradingFunction( | ||
pool.reserves, | ||
pool.totalLiquidity - deltaLiquidity, | ||
getPoolParams(poolId) | ||
); | ||
|
||
valid = invariant >= 0; | ||
} | ||
|
||
/// @inheritdoc IStrategy | ||
function update( | ||
address sender, | ||
|
@@ -105,6 +197,7 @@ contract ConstantSum is PairStrategy { | |
|
||
params.price = internalParams[poolId].price; | ||
params.swapFee = internalParams[poolId].swapFee; | ||
params.controller = internalParams[poolId].controller; | ||
|
||
return abi.encode(params); | ||
} | ||
|
@@ -128,7 +221,7 @@ contract ConstantSum is PairStrategy { | |
Pool memory, | ||
bytes memory | ||
) internal pure override returns (uint256[] memory) { | ||
return new uint256[](0); | ||
return new uint256[](2); | ||
} | ||
|
||
/// @inheritdoc PairStrategy | ||
|
@@ -137,6 +230,20 @@ contract ConstantSum is PairStrategy { | |
Pool memory, | ||
bytes memory | ||
) internal pure override returns (uint256[] memory) { | ||
return new uint256[](0); | ||
return new uint256[](2); | ||
} | ||
|
||
/// @inheritdoc PairStrategy | ||
function _computeSwapDeltaLiquidity( | ||
Pool memory, | ||
bytes memory params, | ||
uint256 tokenInIndex, | ||
uint256, | ||
uint256 amountIn, | ||
uint256 | ||
) internal pure override returns (uint256) { | ||
return computeSwapDeltaLiquidity( | ||
amountIn, abi.decode(params, (ConstantSumParams)), tokenInIndex == 0 | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,15 +6,16 @@ import { ConstantSumParams } from "src/ConstantSum/ConstantSum.sol"; | |
import { ONE } from "src/lib/StrategyLib.sol"; | ||
|
||
using FixedPointMathLib for uint256; | ||
using FixedPointMathLib for int256; | ||
|
||
function computeTradingFunction( | ||
uint256[] memory reserves, | ||
uint256 totalLiquidity, | ||
uint256 price | ||
) pure returns (int256) { | ||
return int256(reserves[0].divWadUp(totalLiquidity)) | ||
+ int256(reserves[1].divWadUp(totalLiquidity.mulWadUp(price))) - int256(ONE); | ||
return int256( | ||
price.mulWadUp(reserves[0].divWadUp(totalLiquidity)) | ||
+ reserves[1].divWadUp(totalLiquidity) | ||
) - int256(ONE); | ||
} | ||
|
||
function computeInitialPoolData( | ||
|
@@ -24,77 +25,28 @@ function computeInitialPoolData( | |
) pure returns (bytes memory) { | ||
// The pool can be initialized with any non-negative amount of rx, and ry. | ||
// so we have to allow a user to pass an amount of both even if one is zero. | ||
uint256 L = rx + ry.divWadUp(params.price); | ||
uint256[] memory reserves = new uint256[](2); | ||
reserves[0] = rx; | ||
reserves[1] = ry; | ||
return abi.encode(reserves, L, params); | ||
return abi.encode(reserves, params); | ||
} | ||
|
||
function computeDeallocateGivenDeltaX( | ||
function computeDeltaLiquidity( | ||
uint256 deltaX, | ||
uint256 rX, | ||
uint256 rY, | ||
uint256 totalLiquidity | ||
) pure returns (uint256 deltaY, uint256 deltaL) { | ||
uint256 a = deltaX.divWadDown(rX); | ||
if (rY > 0) { | ||
deltaY = a.mulWadDown(rY); | ||
} | ||
deltaL = a.mulWadDown(totalLiquidity); | ||
} | ||
|
||
function computeDeallocateGivenDeltaY( | ||
uint256 deltaY, | ||
uint256 rX, | ||
uint256 rY, | ||
uint256 totalLiquidity | ||
) pure returns (uint256 deltaX, uint256 deltaL) { | ||
uint256 a = deltaY.divWadDown(rY); | ||
if (rX > 0) { | ||
deltaX = a.mulWadDown(rX); | ||
} | ||
deltaL = a.mulWadDown(totalLiquidity); | ||
} | ||
|
||
function computeAllocateGivenDeltaX( | ||
uint256 deltaX, | ||
uint256 rX, | ||
uint256 rY, | ||
uint256 totalLiquidity | ||
) pure returns (uint256 deltaY, uint256 deltaL) { | ||
uint256 a = deltaX.divWadUp(rX); | ||
if (rY > 0) { | ||
deltaY = a.mulWadUp(rY); | ||
} | ||
deltaL = a.mulWadUp(totalLiquidity); | ||
} | ||
|
||
function computeAllocateGivenDeltaY( | ||
uint256 deltaY, | ||
uint256 rX, | ||
uint256 rY, | ||
uint256 totalLiquidity | ||
) pure returns (uint256 deltaX, uint256 deltaL) { | ||
uint256 a = deltaY.divWadUp(rY); | ||
if (rX > 0) { | ||
deltaX = a.mulWadUp(rX); | ||
} | ||
deltaL = a.mulWadUp(totalLiquidity); | ||
} | ||
|
||
function computeDeltaGivenDeltaLRoundUp( | ||
uint256 reserve, | ||
uint256 deltaLiquidity, | ||
uint256 totalLiquidity | ||
uint256 price | ||
) pure returns (uint256) { | ||
return reserve.mulWadUp(deltaLiquidity.divWadUp(totalLiquidity)); | ||
return price.mulWadUp(deltaX) + deltaY; | ||
} | ||
|
||
function computeDeltaGivenDeltaLRoundDown( | ||
uint256 reserve, | ||
uint256 deltaLiquidity, | ||
uint256 totalLiquidity | ||
function computeSwapDeltaLiquidity( | ||
uint256 delta, | ||
ConstantSumParams memory params, | ||
bool isSwapXForY | ||
) pure returns (uint256) { | ||
return reserve.mulWadDown(deltaLiquidity.divWadDown(totalLiquidity)); | ||
if (isSwapXForY) { | ||
return (params.swapFee).mulWadUp(delta); | ||
} else { | ||
return (params.swapFee).mulDivUp(delta, params.price); | ||
} | ||
Comment on lines
+47
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic seems inconsistent with the other functions in this file (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can confirm this, when calling // SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.18;
import "forge-std/Test.sol";
import "src/FixedPointMathLib.sol";
contract SwapTest is Test {
using FixedPointMathLib for uint256;
using FixedPointMathLib for int256;
function testSwapWithFees() public {
// Pool state
uint256 rX = 10e18;
uint256 rY = 20e18;
uint256 price = 2e18;
uint256 swapFee = 0.05e18;
uint256 tL = price.mulWadUp(rX) + rY;
_invariant(rX, rY, tL, price);
// Swap 1 tokenX for 2 tokenY
uint256 amountIn = 1e18;
uint256 amountOut = uint256(2e18).mulWadDown(1e18 - swapFee);
uint256 dL = computeSwapDeltaLiquidity(amountIn, price, swapFee, true);
_invariant(
rX + amountIn,
rY - amountOut,
tL + dL,
price
);
}
function _invariant(
uint256 rX,
uint256 rY,
uint256 tL,
uint256 price
) internal returns (int256) {
int256 invariant = int256(price.mulWadUp(rX.divWadUp(tL)) + rY.divWadUp(tL)) - int256(1e18);
console2.log("invariant:", invariant);
}
function computeSwapDeltaLiquidity(
uint256 delta,
uint256 price,
uint256 swapFee,
bool isSwapXForY
) internal returns (uint256) {
if (isSwapXForY) {
return (swapFee).mulWadUp(delta);
} else {
return (swapFee).mulDivUp(delta, price);
}
}
} Output:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed here 6b5a3f5. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,14 +12,14 @@ import { | |
import { | ||
ONE, | ||
computeInitialPoolData, | ||
FixedPointMathLib | ||
FixedPointMathLib, | ||
computeSwapDeltaLiquidity | ||
} from "./ConstantSumMath.sol"; | ||
|
||
contract ConstantSumSolver { | ||
error NotEnoughLiquidity(); | ||
|
||
using FixedPointMathLib for uint256; | ||
using FixedPointMathLib for int256; | ||
|
||
struct Reserves { | ||
uint256 rx; | ||
|
@@ -59,7 +59,7 @@ contract ConstantSumSolver { | |
SimulateSwapState memory state; | ||
|
||
if (swapXIn) { | ||
state.deltaLiquidity = amountIn.mulWadUp(poolParams.swapFee); | ||
computeSwapDeltaLiquidity(amountIn, poolParams, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I think it's an artefact from the past, let's remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function call was deleted here 016b81a. |
||
state.amountOut = amountIn.mulWadDown(poolParams.price).mulWadDown( | ||
ONE - poolParams.swapFee | ||
); | ||
|
@@ -68,8 +68,7 @@ contract ConstantSumSolver { | |
revert NotEnoughLiquidity(); | ||
} | ||
} else { | ||
state.deltaLiquidity = | ||
amountIn.mulWadUp(poolParams.swapFee).divWadUp(poolParams.price); | ||
computeSwapDeltaLiquidity(amountIn, poolParams, false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here as on line 62. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete here too 016b81a. |
||
state.amountOut = (ONE - poolParams.swapFee).mulWadDown(amountIn) | ||
.divWadDown(poolParams.price); | ||
|
||
|
@@ -81,13 +80,9 @@ contract ConstantSumSolver { | |
bytes memory swapData; | ||
|
||
if (swapXIn) { | ||
swapData = abi.encode( | ||
0, 1, amountIn, state.amountOut, state.deltaLiquidity | ||
); | ||
swapData = abi.encode(0, 1, amountIn, state.amountOut); | ||
} else { | ||
swapData = abi.encode( | ||
1, 0, amountIn, state.amountOut, state.deltaLiquidity | ||
); | ||
swapData = abi.encode(1, 0, amountIn, state.amountOut); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I notice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's intentional yes, the |
||
} | ||
|
||
(bool valid,,,,,,) = IStrategy(strategy).validateSwap( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting that
computeDeltaLiquidity()
doesprice.mulWadUp(deltaX)
, which rounds up.As such, the rounding direction here actually favors the user as it rounds away from the slippage parameter (ie. the user declares a minimum amount of
deltaLiquidity
acceptable, but the rounding makesdeltaLiquidity
larger instead of smaller).Contrast this with
validateAllocate()
in other strategies, such asGeometricMean
, where the user provides a maximum acceptable amount oftokenX
andtokenY
that needs to be transferred into the pool, and the amount of tokens calculated rounds up towards the maximum amount specified:The effect of this is that you mint 1 wei extra of
deltaLiquidity
for the amount oftokenX
transferred in. However, I don't think there's any serious impact sincereserves
have 18 decimals, and ifdeltaLiquidity
was disproportionately larger than the amount oftokenX
transferred in, the invariant check would just revert.Recommended fix would be to calculate
deltaLiquidity
asprice.mulWadDown(deltaX) + deltaY
for onlyvalidateAllocate()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've introduced another function to compute the liquidity and round up or down accordingly, see: 1e51776.