-
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
Conversation
* fix: NatSpec in computeLGivenX in LogNormalMath * Update src/LogNormal/LogNormalMath.sol --------- Co-authored-by: Matt Czernik <matt.czernik@gmail.com>
…function fix: rounding direction in computeTradingFunction in LogNormalMath
uint256 postBalance = ERC20(token).balanceOf(address(this)); | ||
if (postBalance < preBalance + downscaledAmount) { | ||
revert InvalidTransfer(); | ||
} | ||
} | ||
|
||
if (address(this).balance > 0) { | ||
SafeTransferLib.safeTransferETH(msg.sender, address(this).balance); |
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.
Note that this "unconditional ETH return" still potentially represents a DOS vector for interacting contracts that do not have the ability to receive ETH. I don't think it's likely to be a chronic issue as an attacker would have to keep repeatedly sending ETH each time an EOA or ETH-receiving contract interacted w/the DFMM, racking up gas costs. But I could see it being used to block particular contracts from making successful calls in certain time-sensitive situations (e.g. consider a liquidation flow that will sell collateral into a DFMM pool).
The tradeoff of removing this functionality is the inability to recover ETH sent to the contract. I guess that could be a problem if users are trying to trade via raw ETH and aren't sure the exact amount they need to send, so they are generally sending a bit extra which then becomes a loss for them.
So ultimately I would say I agree with keeping this in so long as supporting raw ETH is a design requirement (although it's a requirement I'd question quite strongly).
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 feel it's best to eliminate this DOS vector if possible. An external integration could be calling init()
, allocate()
or swap()
in a critical function that is expected not to fail. Also, this refund makes it possible for msg.sender
to unexpectedly receive ETH when calling either of the three functions.
Assuming that users can calculate the amount of ETH they are supposed to send, you could replace this with a msg.value == amount
check to prevent users from overpaying.
Alternatively, you could convert the ETH sent into WETH and refund the excess in WETH instead. Some protocols such as GMX use this approach when they need to refund ETH. For example, you could remove this block and modify the branch above to be:
if (token == weth && msg.value >= amount) {
WETH(payable(weth)).deposit{ value: msg.value }();
if (msg.value > amount) {
SafeTransferLib.safeTransfer(ERC20(weth), msg.sender, msg.value - amount);
}
} else {
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.
In the case of a swap, amountIn
is specified so it's known and the caller can make amountIn == msg.value
, but for most allocation functions, there's just a maximum amount. If DFMM.allocate
enforced a minDeltaL
then I think the approach of checking msg.value == amount
would be good, although it limits the design of strategies somewhat.
Refunding in WETH works too, the funds might be unrecoverable if sent to a contract that isn't expecting WETH but at least there's no DOS.
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.
We're thinking about following your advice and removing raw ETH completely.
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.
This _transferFrom
call is in a loop though so we'd have to manage the double spending if using msg.value. What if we just remove the DOS vector where we check the balance? What if we only check the contract balance if the token is weth?
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 guess since we dont have duplicate tokens, we'd only actually hit that branch once in a loop. I'll open a pr
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.
PR: #122
uint256 decimals = ERC20(params.tokens[i]).decimals(); | ||
|
||
if (decimals > 18 || decimals < 6) { | ||
revert InvalidTokenDecimals(); | ||
} | ||
|
||
_transferFrom(params.tokens[i], reserves[i]); | ||
for (uint256 j = i + 1; j < tokensLength; j++) { |
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'll note that you could also wrap with unchecked
here as otherwise the i + 1
operation will be checked for overflow (unnecessarily, given the limit placed on tokens.length
above). Depending on the exact Solidity version used, the loop increments may or may not be checked by default as well (in 0.8.22
they introduced unchecked increments as a feature).
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.
Yes, since we're compiling using 0.8.22
we're good here, but I've wrapped the for
loop with unchecked
so we can still save some gas to initialize j
(cf 50d121a).
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pure
function that returns a value, but the return value isn't being used at all. Looks like state.deltaLiquidity
is never set in this branch.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
The function call was deleted here 016b81a.
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Delete here too 016b81a.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I notice state.deltaLiquidity
is no longer being included in swapData
. Either this is intentional and the function calls I commented on above can be removed, or it is unintentional and deltaLiquidity
should be properly computed and included.
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 intentional yes, the deltaLiquidity
is now calculated by the strategies during a swap.
if (isSwapXForY) { | ||
return (params.swapFee).mulWadUp(delta); | ||
} else { | ||
return (params.swapFee).mulDivUp(delta, params.price); | ||
} |
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.
This logic seems inconsistent with the other functions in this file (e.g. computeDeltaLiquidity
immediately above). Liquidity is in units of Y
and X
quantities need to be multiplied by price. If isSwapXForY == true
, I expect delta
to be an X
amount and thus the liquidity change should be something like (params.swapFee).mulWadUp(delta.mulWadUp(params.price))
, and in the false
case, it should be (params.swapFee).mulWadUp(delta)
as then delta
is a Y
quantity.
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.
Can confirm this, when calling swap()
with the correct input and output amounts, you either blow-up the invariant or it becomes negative. I ran the numbers with this:
// 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:
Logs:
invariant: 0
invariant: 1248439450686643
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.
Fixed here 6b5a3f5.
if (_pools[poolId].controllerFee > 0) { | ||
uint256 fees = | ||
state.deltaLiquidity.mulWadUp(_pools[poolId].controllerFee); | ||
_pools[poolId].totalLiquidity += state.deltaLiquidity - fees; |
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.
What's the reasoning behind subtracting fees
from state.deltaLiquidity
here? If the strategy isn't paying attention to the controllerFee
value, it seems like this could invalidate the strategy's calculations (that said, a smaller liquidity increase should bias proper invariants higher rather than lower, so it shouldn't be exploitable).
The proximate effect is to make the controller's fee share a little higher, couldn't that just be accomplished by increasing the controller fee in practice? It seems "morally" wrong to be modifying values returned by the strategy, which is otherwise trusted as a black box to provide state variable deltas.
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 to ensure the correct amount of LP tokens gets minted to the feeCollector
address. I went through the math in the original finding: https://github.com/cantinasec/review-primitive-excalibur/issues/10
(unless some change in validateSwap()
affected the math, I haven't looked at the strategies yet)
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.
Ah I missed that fees
is being added onto totalLiquidity
below, so the expected state change is still being applied.
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 to ensure the correct amount of LP tokens gets minted to the
feeCollector
address. I went through the math in the original finding: cantinasec/review-primitive-excalibur#10(unless some change in
validateSwap()
affected the math, I haven't looked at the strategies yet)
Hey @kinrezC can we confirm that any of the changes with the computed delta liquidity in validateSwap won't affect this negatively?
@@ -74,14 +88,26 @@ contract LogNormal is PairStrategy { | |||
(reserves, totalLiquidity, params) = | |||
abi.decode(data, (uint256[], uint256, LogNormalParams)); | |||
|
|||
if (params.mean < MIN_WIDTH || params.mean > MAX_MEAN) { |
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.
MIN_WIDTH
should be MIN_MEAN
here.
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.
Fixed here 2c700a6.
} | ||
} | ||
|
||
for (uint256 i = 0; i < tokensLength; i++) { | ||
uint256 decimals = ERC20(params.tokens[i]).decimals(); |
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.
You can use token
here instead of params.tokens[i]
again:
address token = params.tokens[i];
- uint256 decimals = ERC20(params.tokens[i]).decimals();
+ uint256 decimals = ERC20(token).decimals();
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.
Fixed here e1d1a4d.
if (token == weth && address(this).balance >= amount) { | ||
WETH(payable(weth)).deposit{ value: amount }(); | ||
} else { |
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 an attacker can still "pay on behalf" of a victim by directly sending an ETH balance of amount
to the contract, causing this branch to be taken instead of the one below.
Perhaps this might cause problems for certain external integrations that expect their WETH balance to decrease after calling init()
, allocate()
or swap()
. Additionally, it would leave a dangling WETH approval from the calling address to the DFMM
contract.
Is there any reason we don't use msg.value
here instead of address(this).balance
?
- if (token == weth && address(this).balance >= amount) {
+ if (token == weth && msg.value >= amount) {
WETH(payable(weth)).deposit{ value: amount }();
} else {
This would prevent any attacker from interfering with a victim's calls by sending ETH to the contract.
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.
Same comment as above, in the end we might want to get rid of raw ETH completely.
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.
Fix pr: #122
Unrelated to previous issues, but in uint256 downscaledAmount =
downscaleDown(amount, computeScalingFactor(token));
+ if (downscaledAmount == 0) return;
uint256 preBalance = ERC20(token).balanceOf(address(this));
SafeTransferLib.safeTransfer(ERC20(token), to, downscaledAmount); This is because some tokens (eg. LEND) revert when |
(uint256 deltaX, uint256 deltaY, uint256 minDeltaL) = | ||
abi.decode(data, (uint256, uint256, uint256)); | ||
|
||
deltaLiquidity = | ||
computeDeltaLiquidity(deltaX, deltaY, internalParams[poolId].price); | ||
if (deltaLiquidity < minDeltaL) revert InvalidDeltaLiquidity(); |
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.
Is there any reason why ConstantSum.validateAllocate()
was changed to use deltaX
and deltaY
as the amount to allocate, and have minDeltaL
as the slippage parameter?
The parameters for validateAllocate()
in ConstantSum
are now inconsistent with other pair strategies, where deltaL
is the the amount to allocate and maxDeltaX
and maxDeltaY
are the slippage parameters. The same goes for ConstantSum.validateDeallocate()
as well.
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 comment
The 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 ConstantSum
we had to change that because we need both deltas to compute deltaLiquidity
.
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 see, in that case I don't think this inconsistency is avoidable.
deltaLiquidity = | ||
computeDeltaLiquidity(deltaX, deltaY, internalParams[poolId].price); | ||
if (maxDeltaL > deltaLiquidity) revert InvalidDeltaLiquidity(); |
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.
This check is reversed, the function should be ensuring that deltaLiquidity
is not greater than maxDeltaL
:
- if (maxDeltaL > deltaLiquidity) revert InvalidDeltaLiquidity();
+ if (maxDeltaL < deltaLiquidity) revert InvalidDeltaLiquidity();
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.
Fixed here 13aa509.
That's a good point, I'll make that change (Cf c086b1b). |
uint256 a = uint256(int256(rX.divWadDown(L)).powWad(int256(params.wX))); | ||
uint256 b = uint256(int256(rY.divWadDown(L)).powWad(int256(params.wY))); | ||
uint256 a = uint256(int256(rX.divWadUp(L)).powWad(int256(params.wX))); | ||
uint256 b = uint256(int256(rY.divWadUp(L)).powWad(int256(params.wY))); | ||
|
||
return int256(a.mulWadUp(b)) - int256(1 ether); |
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.
Consider using ONE
here instead of 1 ether
.
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.
Indeed, here is the fix: 1009b0f.
uint256 a = uint256( | ||
int256(reserves[i].divWadDown(L)).powWad(int256(params.weights[i])) | ||
); |
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.
Would recommend rounding up reserves[i].divWadDown(L)
here as well to be consistent with G3MMath.computeTradingFunction()
:
uint256 a = uint256(int256(rX.divWadUp(L)).powWad(int256(params.wX)));
uint256 b = uint256(int256(rY.divWadUp(L)).powWad(int256(params.wY)));
Recommendation:
uint256 a = uint256(
- int256(reserves[i].divWadDown(L)).powWad(int256(params.weights[i]))
+ int256(reserves[i].divWadUp(L)).powWad(int256(params.weights[i]))
);
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.
Needs to be fixed still @clemlak
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.
Fixed here cc42a4e.
return weight.mulWadDown(swapFee).mulWadDown(totalLiquidity).mulWadDown( | ||
amountIn.divWadDown(reserve) | ||
); |
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.
Is there any reason why G3MMath.computeSwapDeltaLiquidity()
rounds up, but the calculation for deltaLiquidity
here rounds down?
return weight.mulWadUp(swapFee).mulWadUp(totalLiquidity).mulWadUp(
amountIn.divWadUp(reserve)
);
I don't think there's a correct direction to round towards, but I'm inclined to think that rounding up is safer as deltaLiquidity
would round in favor of the protocol and against the user.
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.
Yes, obviously, fix is here: 039093f.
The fix for https://github.com/cantinasec/review-primitive-excalibur/issues/20 hasn't been applied to NTokenGeometricMeanMath.sol#L28-L42. |
deltaLiquidity = | ||
computeDeltaLiquidity(deltaX, deltaY, internalParams[poolId].price); |
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()
does price.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 makes deltaLiquidity
larger instead of smaller).
Contrast this with validateAllocate()
in other strategies, such as GeometricMean
, where the user provides a maximum acceptable amount of tokenX
and tokenY
that needs to be transferred into the pool, and the amount of tokens calculated rounds up towards the maximum amount specified:
function _computeAllocateDeltasGivenDeltaL(
uint256 deltaLiquidity,
Pool memory pool,
bytes memory
) internal pure override returns (uint256[] memory deltas) {
deltas = new uint256[](2);
deltas[0] = computeDeltaGivenDeltaLRoundUp(
pool.reserves[0], deltaLiquidity, pool.totalLiquidity
);
deltas[1] = computeDeltaGivenDeltaLRoundUp(
pool.reserves[1], deltaLiquidity, pool.totalLiquidity
);
}
The effect of this is that you mint 1 wei extra of deltaLiquidity
for the amount of tokenX
transferred in. However, I don't think there's any serious impact since reserves
have 18 decimals, and if deltaLiquidity
was disproportionately larger than the amount of tokenX
transferred in, the invariant check would just revert.
Recommended fix would be to calculate deltaLiquidity
as price.mulWadDown(deltaX) + deltaY
for only validateAllocate()
.
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.
Done here 754916c. |
This branch contains all the fixes for the different findings discovered during our audit with Spearbit.