QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: medium
Valid

QA

hardlydifficult

1) [LOW] When owner updates are enabled, admin permissions are ignored

UpdateWeightRunner.sol uses a bitmask for poolRuleSettings which authorize actions such as setWeightsManually. As a bitmask, both the MASK_POOL_OWNER_UPDATES and MASK_POOL_QUANTAMM_ADMIN_UPDATES may be enabled at the same time. It seems like allowing both would be desirable.

However the implementations checking authorization will only consider the poolManager when MASK_POOL_OWNER_UPDATES:

if (poolRegistryEntry & MASK_POOL_OWNER_UPDATES > 0) {
require(msg.sender == poolRuleSettings[_poolAddress].poolManager, "ONLYMANAGER");
} else if (poolRegistryEntry & MASK_POOL_QUANTAMM_ADMIN_UPDATES > 0) {
require(msg.sender == quantammAdmin, "ONLYADMIN");
} else {
revert("No permission to set last run time");
}

Source: UpdateWeightRunner.sol#L317-L321 (plus 2 other instances)

Recommendation: Update these checks to allow calls from either the poolManager or quantammAdmin when both flags are enabled. e.g.

diff --git a/pkg/pool-quantamm/contracts/UpdateWeightRunner.sol b/pkg/pool-quantamm/contracts/UpdateWeightRunner.sol
index 6795089..f93c1ea 100644
--- a/pkg/pool-quantamm/contracts/UpdateWeightRunner.sol
+++ b/pkg/pool-quantamm/contracts/UpdateWeightRunner.sol
@@ -315,7 +315,10 @@ contract UpdateWeightRunner is Ownable2Step, IUpdateWeightRunner {
//current breakglass settings allow pool creator trigger. This is subject to review
if (poolRegistryEntry & MASK_POOL_OWNER_UPDATES > 0) {
- require(msg.sender == poolRuleSettings[_poolAddress].poolManager, "ONLYMANAGER");
+ require(
+ msg.sender == poolRuleSettings[_poolAddress].poolManager
+ || (poolRegistryEntry & MASK_POOL_QUANTAMM_ADMIN_UPDATES > 0 && msg.sender == quantammAdmin)
+ , "ONLYMANAGER");
} else if (poolRegistryEntry & MASK_POOL_QUANTAMM_ADMIN_UPDATES > 0) {
require(msg.sender == quantammAdmin, "ONLYADMIN");
} else {

2) [LOW] UpdateWeightRunner inherits Ownable2Step but does not use owner

UpdateWeightRunner.sol inherits Ownable2Step however it does not grant the owner account any permissions. Instead a different variable is used: quantammAdmin.

Recommendation: Either remove Ownable2Step to avoid confusion or merge the variables so they use the same data slot (effectively just branding the "owner" to a more meaninful name), e.g.:

function quantammAdmin() public returns (address) {
return owner();
}

3) [LOW] setQuantAMMSwapFeeTake and setQuantAMMUpliftFeeTake impact the same data slot

UpdateWeightRunner.sol has 2 functions which sound like they offer granular control over fees - however they share the same variable. So changing the "uplift fee" also changes the "swap fee" and vice versa. The functions and the events emitted imply separate configurations.

Recommendation: Either collapse into a single setQuantAMMFeeTake (for both swap and uplift) to avoid confusion, or change the implementation to use separate data slots for each, e.g.:

diff --git a/pkg/pool-quantamm/contracts/UpdateWeightRunner.sol b/pkg/pool-quantamm/contracts/UpdateWeightRunner.sol
index 6795089..72d2a06 100644
--- a/pkg/pool-quantamm/contracts/UpdateWeightRunner.sol
+++ b/pkg/pool-quantamm/contracts/UpdateWeightRunner.sol
@@ -122,6 +122,7 @@ contract UpdateWeightRunner is Ownable2Step, IUpdateWeightRunner {
/// @notice The % of the total swap fee that is allocated to the protocol for running costs.
uint256 public quantAMMSwapFeeTake = 0.5e18;
+ uint256 public quantAMMUpliftFeeTake = 0.5e18;
function setQuantAMMSwapFeeTake(uint256 _quantAMMSwapFeeTake) external override {
require(msg.sender == quantammAdmin, "ONLYADMIN");
@@ -141,15 +142,15 @@ contract UpdateWeightRunner is Ownable2Step, IUpdateWeightRunner {
function setQuantAMMUpliftFeeTake(uint256 _quantAMMUpliftFeeTake) external{
require(msg.sender == quantammAdmin, "ONLYADMIN");
require(_quantAMMUpliftFeeTake <= 1e18, "Uplift fee must be less than 100%");
- uint256 oldSwapFee = quantAMMSwapFeeTake;
- quantAMMSwapFeeTake = _quantAMMUpliftFeeTake;
+ uint256 oldSwapFee = quantAMMUpliftFeeTake;
+ quantAMMUpliftFeeTake = _quantAMMUpliftFeeTake;
emit UpliftFeeTakeSet(oldSwapFee, _quantAMMUpliftFeeTake);
}
/// @notice Get the quantAMM uplift fee % amount allocated to the protocol for running costs
function getQuantAMMUpliftFeeTake() external view returns (uint256){
- return quantAMMSwapFeeTake;
+ return quantAMMUpliftFeeTake;
}

4) [LOW] nftPool is not deleted after remove liquidity

Tests for UpliftOnlyExample.sol include the following assertion after removing liquidity:

assertEq(upliftOnlyRouter.nftPool(nftTokenId), address(0), "pool mapping should be 0");

Source UpliftExample.t.sol#L518 (plus an additional 4 tests with the same assertion).

However the tests are invalid due to an incorrect tokenId. The NFT tokenIds start with 1 (tokenID 0 is not valid).

uint256 nftTokenId = 0;

Source UpliftExample.t.sol#L455 (& others)

If you fix the tokenId (uint256 nftTokenId = 1;), tests will fail.

Recommendation: It is trivial to add the logic this is testing for, and doing so would reduce gas costs as well:

diff --git a/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol b/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol
index fbf4f56..adafc0d 100644
--- a/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol
+++ b/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol
@@ -497,6 +497,7 @@ contract UpliftOnlyExample is MinimalRouter, BaseHooks, Ownable {
localData.amountLeft -= feeDataArray[i].amount;
lpNFT.burn(feeDataArray[i].tokenID);
+ delete nftPool(feeDataArray[i].tokenID);
delete feeDataArray[i];
feeDataArray.pop();

5) Non-critical / informational

  • QuantAMMStorage.sol#L214-L222 has a for loop which can be collapsed into a single line for better readability and lower gas costs: nonStickySourceLength = _sourceArray.length - (_sourceArray.length % 8);

  • QuantAMMWeightedPoolFactory.sol#L60 _poolVersion cannot be changed and therefor can be made immutable.

  • Spelling errors appear in the external interface. I recommend using CSpell which you can configure to target just .sol files and after adding a few custom terms such as quantamm it can become a required step in CI. Example spelling errors: event OracleRemved and error TransferUpdateTokenIDInvaid

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_quantAMMSwapFeeTake==quantAMMUplfitFeeTake

Likelyhood: High, calling setters or getters Impact: Low/Medium, both getters return `quantAMMSwapFeeTake` and `setQuantAMMUpliftFeeTake` modify `quantAMMUplfitFeeTake`. Real impact: those 2 values will be always the same.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!