Skip to content
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.

Sign up for GitHub

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

fix: pool domination attack #17

Merged
merged 6 commits into from
Nov 30, 2023
Merged

fix: pool domination attack #17

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a67e19b
fix: pool domination attack
kosecki123 Nov 27, 2023
516c1f8
fix: negative fees accounted as fixed fee
kosecki123 Nov 27, 2023
e62518a
Merge branch 'main' into fix/pool-domination-attack
kosecki123 Nov 28, 2023
c963782
chore: forge fmt
kosecki123 Nov 28, 2023
2a147b9
fix: adjust test so I accounts for dust redemptions
kosecki123 Nov 29, 2023
3a8aa6d
fix: grammar
kosecki123 Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 24 additions & 24 deletions src/FeeCalculator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// If you encounter a vulnerability or an issue, please contact <info@neutralx.com>
pragma solidity ^0.8.13;

import "forge-std/console.sol";
import "./interfaces/IDepositFeeCalculator.sol";
import "./interfaces/IRedemptionFeeCalculator.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
Expand Down Expand Up @@ -54,9 +55,9 @@ contract FeeCalculator is IDepositFeeCalculator, IRedemptionFeeCalculator {
/// @return recipients The addresses of the fee recipients.
/// @return feesDenominatedInPoolTokens The amount of fees each recipient should receive.
function calculateDepositFees(address tco2, address pool, uint256 depositAmount)
external
kosecki123 marked this conversation as resolved.
Show resolved Hide resolved
override
returns (address[] memory recipients, uint256[] memory feesDenominatedInPoolTokens)
external
override
returns (address[] memory recipients, uint256[] memory feesDenominatedInPoolTokens)
{
require(depositAmount > 0, "depositAmount must be > 0");

Expand All @@ -73,9 +74,9 @@ contract FeeCalculator is IDepositFeeCalculator, IRedemptionFeeCalculator {
/// @return recipients The addresses of the fee recipients.
/// @return feesDenominatedInPoolTokens The amount of fees each recipient should receive.
function distributeFeeAmongShares(uint256 totalFee)
private
view
returns (address[] memory recipients, uint256[] memory feesDenominatedInPoolTokens)
private
view
returns (address[] memory recipients, uint256[] memory feesDenominatedInPoolTokens)
{
feesDenominatedInPoolTokens = new uint256[](_recipients.length);

Expand All @@ -98,9 +99,9 @@ contract FeeCalculator is IDepositFeeCalculator, IRedemptionFeeCalculator {
/// @return recipients The addresses of the fee recipients.
/// @return feesDenominatedInPoolTokens The amount of fees each recipient should receive.
function calculateRedemptionFee(address tco2, address pool, uint256 depositAmount)
external
override
returns (address[] memory recipients, uint256[] memory feesDenominatedInPoolTokens)
external
override
returns (address[] memory recipients, uint256[] memory feesDenominatedInPoolTokens)
{
require(depositAmount > 0, "depositAmount must be > 0");

Expand Down Expand Up @@ -147,9 +148,9 @@ contract FeeCalculator is IDepositFeeCalculator, IRedemptionFeeCalculator {
/// @param total The total supply of the pool.
/// @return The calculated ratios.
function getRatiosRedemption(SD59x18 amount, SD59x18 current, SD59x18 total)
private
view
returns (SD59x18, SD59x18)
private
view
returns (SD59x18, SD59x18)
{
SD59x18 a = total == zero ? zero : current / total;
SD59x18 b = (total - amount) == zero ? zero : (current - amount) / (total - amount);
Expand Down Expand Up @@ -198,31 +199,30 @@ contract FeeCalculator is IDepositFeeCalculator, IRedemptionFeeCalculator {
require(amount <= current);

SD59x18 amount_float = sd(int256(amount));
SD59x18 ta = sd(int256(current));
0xmichalis marked this conversation as resolved.
Show resolved Hide resolved
SD59x18 tb = ta - amount_float;

(SD59x18 da, SD59x18 db) = getRatiosRedemption(amount_float, ta, sd(int256(total)));

console.logUint(intoUint256(da));
console.logUint(intoUint256(db));
console.logUint(intoUint256(da-db));

if (
current == total //single asset (or no assets) special case
current == total //single asset (or no assets)
) {
uint256 fee = intoUint256(amount_float * (singleAssetRedemptionRelativeFee));
return fee;
}

SD59x18 ta = sd(int256(current));
SD59x18 tb = ta - amount_float;

(SD59x18 da, SD59x18 db) = getRatiosRedemption(amount_float, ta, sd(int256(total)));

//redemption_fee = scale * (tb * log10(b+shift) - ta * log10(a+shift)) + constant*amount;
SD59x18 i_a = ta * (da + redemptionFeeShift).log10();
SD59x18 i_b = tb * (db + redemptionFeeShift).log10();
SD59x18 fee_float = redemptionFeeScale * (i_b - i_a) + redemptionFeeConstant * amount_float;

if (fee_float < zero) {
if (fee_float / amount_float < sd(1e-6 * 1e18)) {
//fee_float=zero_signed;//if the fee is negative but is less than 0.0001% of amount than it's basically 0
require(fee_float > zero, "Fee must be greater than 0");
} else {
require(fee_float > zero, "Total failure. Fee must be greater than 0 or at least close to it.");
}
uint256 fee = intoUint256(amount_float * (singleAssetRedemptionRelativeFee));
0xmichalis marked this conversation as resolved.
Show resolved Hide resolved
return fee;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, is this ok if it rounds down to zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In such case, because of

require(totalFee > 0, "Fee must be greater than 0");

protocol will fail

This means we cannot support redemption of amount <= 10 from the highly dominated pool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kosecki123 when you say amount <= 10 that is in wei, right?

}

uint256 fee = intoUint256(fee_float);
Expand Down
82 changes: 37 additions & 45 deletions test/FeeCalculator.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,51 +170,6 @@ contract FeeCalculatorTest is Test {
assertEq(fees[0], redemptionAmount / 10);
}

function testCalculateRedemptionFees_AlmostFullMonopolization_ZeroFees() public {
// Arrange
// Set up your test data
uint256 redemptionAmount = 1 * 1e18;

// Set up mock pool
mockPool.setTotalSupply(1e6 * 1e18 + 1);
mockToken.setTokenBalance(address(mockPool), 1e6 * 1e18);

// Act
try feeCalculator.calculateRedemptionFee(address(mockToken), address(mockPool), redemptionAmount) returns (
address[] memory recipients, uint256[] memory fees
) {
// Assert
assertEq(recipients[0], feeRecipient);
assertEq(fees[0], 0);
fail("Exception should be thrown");
} catch Error(string memory reason) {
assertEq("Fee must be greater than 0", reason);
}
}

function testCalculateRedemptionFees_CurrentSlightLessThanTotal_AmountSuperSmall_ShouldResultInException() public {
//this test was producing negative redemption fees before rounding extremely small negative redemption fees to zero
// Arrange
// Set up your test data
uint256 redemptionAmount = 186843141273221600445448244614; //1.868e29

// Set up mock pool
mockPool.setTotalSupply(11102230246251565404236316680908203126); //1.11e37
mockToken.setTokenBalance(address(mockPool), 11102230246251565403820829061134812052); //1.11e37

// Act
try feeCalculator.calculateRedemptionFee(address(mockToken), address(mockPool), redemptionAmount) returns (
address[] memory recipients, uint256[] memory fees
) {
// Assert
assertEq(recipients[0], feeRecipient);
assertEq(fees[0], 0);
fail("Exception should be thrown");
} catch Error(string memory reason) {
assertEq("Fee must be greater than 0", reason);
}
}

function testCalculateDepositFeesNormalCase_TwoFeeRecipientsSplitEqually() public {
// Arrange
// Set up your test data
Expand Down Expand Up @@ -646,6 +601,43 @@ contract FeeCalculatorTest is Test {
assertEq(fees[0], 737254938220315128);
}

function testCalculateRedemptionFees_HugeTotalLargeCurrentSmallDeposit() public {
// Arrange
// Set up your test data
uint256 depositAmount = 10000 * 1e18;

// Set up mock pool
uint256 supply = 100000 * 1e18;
mockPool.setTotalSupply(100000 * 1e18);
mockToken.setTokenBalance(address(mockPool), supply-1);

// Act
(address[] memory recipients, uint256[] memory fees) =
feeCalculator.calculateRedemptionFee(address(mockToken), address(mockPool), depositAmount);

// Assert
assertEq(recipients[0], feeRecipient);
assertEq(fees[0], depositAmount/10);
}

function testCalculateRedemptionFees_NegativeCase() public {
// Arrange
// Set up your test data
uint256 depositAmount = 2323662174650;

// Set up mock pool
mockPool.setTotalSupply(56636794628913227180683983236);
mockToken.setTokenBalance(address(mockPool), 55661911070827884041095553095);

// Act
(address[] memory recipients, uint256[] memory fees) =
feeCalculator.calculateRedemptionFee(address(mockToken), address(mockPool), depositAmount);

// Assert
assertEq(recipients[0], feeRecipient);
assertEq(fees[0], depositAmount/10);
}

function testCalculateDepositFeesFuzzy(uint256 depositAmount, uint256 current, uint256 total) public {
//vm.assume(depositAmount > 0);
//vm.assume(total > 0);
Expand Down