QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: low
Invalid

if a pool allows both `MASK_POOL_OWNER_UPDATES` and `MASK_POOL_QUANTAMM_ADMIN_UPDATES`, the `quantammAdmin` will still be unable to call the function `InitialisePoolLastRunTime`, `setIntermediateValuesManually` and `setWeightsManually`

biakia

Summary

The MASK_POOL_OWNER_UPDATES is a mask to check if a pool owner can update weights and the MASK_POOL_QUANTAMM_ADMIN_UPDATES is a mask to check if a pool is allowed to perform admin updates. if a pool allows both MASK_POOL_OWNER_UPDATES and MASK_POOL_QUANTAMM_ADMIN_UPDATES, the quantammAdmin will still be unable to call the function InitialisePoolLastRunTime , setIntermediateValuesManually and setWeightsManually functions due to logical flaw in these two functions.

Vulnerability Details

Both InitialisePoolLastRunTime , setIntermediateValuesManually and setWeightsManually functions have following privilege checking:

function InitialisePoolLastRunTime(address _poolAddress, uint40 _time) external {
uint256 poolRegistryEntry = approvedPoolActions[_poolAddress];
​
//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");
} else if (poolRegistryEntry & MASK_POOL_QUANTAMM_ADMIN_UPDATES > 0) {
require(msg.sender == quantammAdmin, "ONLYADMIN");
} else {
revert("No permission to set last run time");
}
function setWeightsManually(
int256[] calldata _weights,
address _poolAddress,
uint40 _lastInterpolationTimePossible,
uint _numberOfAssets
) external {
uint256 poolRegistryEntry = QuantAMMWeightedPool(_poolAddress).poolRegistry();
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 weight values");
}
function setIntermediateValuesManually(
address _poolAddress,
int256[] memory _newMovingAverages,
int256[] memory _newParameters,
uint _numberOfAssets
) external {
uint256 poolRegistryEntry = approvedPoolActions[_poolAddress];
​
//Who can trigger these very powerful breakglass features is under review
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 intermediate values");
}

The issue is that if a pool allows both MASK_POOL_OWNER_UPDATES and MASK_POOL_QUANTAMM_ADMIN_UPDATES, the functions will always enter the first if condition:

if (poolRegistryEntry & MASK_POOL_OWNER_UPDATES > 0) {
require(msg.sender == poolRuleSettings[_poolAddress].poolManager, "ONLYMANAGER");
}

which checks for the pool manager, thus potentially excluding the quantammAdmin from accessing the function. This leads to a situation where the quantammAdmin is unable to execute the function even when they rightfully should have access based on their permissions.

POC

Add following test case in UpdateWeightRunnerTest:

function testSetWeightsManuallyAdminShouldPass() public {
int256[] memory weights = new int256[]();
weights[0] = 0.0000000005e18;
weights[1] = 0.0000000005e18;
weights[2] = 0;
weights[3] = 0;
// allow both pool owner and admin to run the function
mockPool.setPoolRegistry(8+16);
​
vm.startPrank(owner);
// allow both pool owner and admin to run the function
updateWeightRunner.setApprovedActionsForPool(address(mockPool), 8+16);
vm.stopPrank();
uint40 blockTime = uint40(block.timestamp);
int216 fixedValue = 1000;
uint delay = 3600;
chainlinkOracle = deployOracle(fixedValue, delay);
​
vm.startPrank(owner);
updateWeightRunner.addOracle(OracleWrapper(chainlinkOracle));
vm.stopPrank();
​
vm.startPrank(address(mockPool));
address[][] memory oracles = new address[][]();
oracles[0] = new address[]();
oracles[0][0] = address(chainlinkOracle);
​
uint64[] memory lambda = new uint64[]();
lambda[0] = 0.0000000005e18;
updateWeightRunner.setRuleForPool(
IQuantAMMWeightedPool.PoolSettings({
assets: new IERC20[](0),
rule: mockRule,
oracles: oracles,
updateInterval: 1,
lambda: lambda,
epsilonMax: 0.2e18,
absoluteWeightGuardRail: 0.2e18,
maxTradeSizeRatio: 0.2e18,
ruleParameters: new int256[][](),
poolManager: addr2
})
);
vm.stopPrank();
​
vm.startPrank(addr2);
updateWeightRunner.InitialisePoolLastRunTime(address(mockPool), blockTime);
vm.stopPrank();
//owner call setWeightsManually and it should pass
vm.startPrank(owner);
updateWeightRunner.setWeightsManually(weights, address(mockPool), 6, 2);
vm.stopPrank();
uint256[] memory poolWeights = new uint256[]();
poolWeights[0] = 0.0000000005e18;
poolWeights[1] = 0.0000000005e18;
assertEq(IWeightedPool(address(mockPool)).getNormalizedWeights(), poolWeights);
}
​

The pool sets mask to 8+16 , which allows both pool owner and admin to run the function. The admin try to call setWeightsManually :

//owner call setWeightsManually and it should pass
vm.startPrank(owner);
updateWeightRunner.setWeightsManually(weights, address(mockPool), 6, 2);
vm.stopPrank();

run forge test -vvv --match-test testSetWeightsManuallyAdminShouldPass :

Ran 1 test for test/foundry/UpdateWeightRunner.t.sol:UpdateWeightRunnerTest
[FAIL: revert: ONLYMANAGER] testSetWeightsManuallyAdminShouldPass() (gas: 560366)
Traces:
[560366] UpdateWeightRunnerTest::testSetWeightsManuallyAdminShouldPass()
├─ [22421] MockQuantAMMBasePool::setPoolRegistry(24)
│ └─ ← [Stop]
├─ [0] VM::startPrank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
│ └─ ← [Return]
├─ [24476] MockUpdateWeightRunner::setApprovedActionsForPool(MockQuantAMMBasePool: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 24)
│ ├─ emit SetApprovedActionsForPool(caller: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, pool: MockQuantAMMBasePool: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], actions: 24)
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [141530] → new MockChainlinkOracle@0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
│ └─ ← [Return] 595 bytes of code
├─ [0] VM::startPrank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
│ └─ ← [Return]
├─ [24083] MockUpdateWeightRunner::addOracle(MockChainlinkOracle: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
│ ├─ emit OracleAdded(oracleAddress: MockChainlinkOracle: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a])
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(MockQuantAMMBasePool: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
│ └─ ← [Return]
├─ [260256] MockUpdateWeightRunner::setRuleForPool(PoolSettings({ assets: [], rule: 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f, oracles: [[0xF62849F9A0B5Bf2913b396098F7c7019b51A820a]], updateInterval: 1, lambda: [500000000 [5e8]], epsilonMax: 200000000000000000 [2e17], absoluteWeightGuardRail: 200000000000000000 [2e17], maxTradeSizeRatio: 200000000000000000 [2e17], ruleParameters: [], poolManager: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69 }))
│ ├─ emit PoolRuleSet(rule: MockIdentityRule: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], poolOracles: [[0xF62849F9A0B5Bf2913b396098F7c7019b51A820a]], lambda: [500000000 [5e8]], ruleParameters: [], epsilonMax: 200000000000000000 [2e17], absoluteWeightGuardRail: 200000000000000000 [2e17], updateInterval: 1, poolManager: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
│ └─ ← [Return]
├─ [2579] MockUpdateWeightRunner::InitialisePoolLastRunTime(MockQuantAMMBasePool: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1)
│ ├─ emit PoolLastRunSet(poolAddress: MockQuantAMMBasePool: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], time: 1)
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
│ └─ ← [Return]
├─ [1767] MockUpdateWeightRunner::setWeightsManually([500000000 [5e8], 500000000 [5e8], 0, 0], MockQuantAMMBasePool: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 6, 2)
│ ├─ [351] MockQuantAMMBasePool::poolRegistry() [staticcall]
│ │ └─ ← [Return] 24
│ └─ ← [Revert] revert: ONLYMANAGER
└─ ← [Revert] revert: ONLYMANAGER
​
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.46ms (152.67µs CPU time)

The setWeightsManually function fails due to incorrect privilege checks.

Impact

The Impact is MEDIUM because the admin is unable to execute the function even when they rightfully should have access based on their permissions, the Likelihood is MEDIUM so the Severity is MEDIUM.

Tools Used

Manual Review

Recommendations

Consider following fix:

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

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Design choice
Assigned finding tags:

invalid_manual_functions_cannot_be_called_by_admin_when_pool_owner_can

Design choice confirmed by the sponsor.

Support

FAQs

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

Give us feedback!