Summary
The UpliftOnlyExample contract has unsanitized params in the constructor. As it can be seen in the code snippet below, the constructor of the contract accepts a _upliftFeeBps, _minWithdrawalFeeBps, _updateWeightRunnerParam, and Version, and so on. These four parameters are important parameters for the contract, and they are used in the contract's logic. However, the contract does not check the validity of these parameters. If the contract is deployed with invalid parameters, it could disrupt the contract's intended functionality. Where an empty string value for version could be a potential issue and the parameters _upliftFeeBps, and _minWithdrawalFeeBps are used to evaluate fee per LP (Liquidity Provider) and if these values are set to zero or to some other undesired hostile value, it could disrupt the contract's intended functionality. Moreover, same for _updateWeightRunnerParam which is used to update the weight of the pool.
Vulnerability Details
see the code snippet below:
UpliftOnlyExample.sol::constructor:
constructor(
IVault vault,
IWETH weth,
IPermit2 permit2,
uint64 _upliftFeeBps,
uint64 _minWithdrawalFeeBps,
address _updateWeightRunnerParam,
string memory version,
string memory name,
string memory symbol
) MinimalRouter(vault, weth, permit2, version) Ownable(msg.sender) {
require(bytes(name).length > 0 && bytes(symbol).length > 0, "NAMEREQ");
lpNFT = new LPNFT(name, symbol, address(this));
upliftFeeBps = _upliftFeeBps;
minWithdrawalFeeBps = _minWithdrawalFeeBps;
_updateWeightRunner = _updateWeightRunnerParam;
}
Also, there should have a min and max value check for the _upliftFeeBps, _minWithdrawalFeeBps parameters.
PoC
The following code snippet demonstrates how the contract could be deployed with invalid parameters:
Steps to reproduce:
-
Go to the test folder inside pool-hooks folder then -> foundry folder -> UpliftExample.t.sol
-
Replace the setUp function with the following code snippet:
function setUp() public virtual override {
BaseTest.setUp();
(address ownerLocal, address addr1Local, address addr2Local) = (vm.addr(1), vm.addr(2), vm.addr(3));
owner = ownerLocal;
addr1 = addr1Local;
addr2 = addr2Local;
vault = deployVaultMock();
vm.label(address(vault), "vault");
vaultExtension = IVaultExtension(vault.getVaultExtension());
vm.label(address(vaultExtension), "vaultExtension");
vaultAdmin = IVaultAdmin(vault.getVaultAdmin());
vm.label(address(vaultAdmin), "vaultAdmin");
authorizer = BasicAuthorizerMock(address(vault.getAuthorizer()));
vm.label(address(authorizer), "authorizer");
factoryMock = PoolFactoryMock(address(vault.getPoolFactoryMock()));
vm.label(address(factoryMock), "factory");
router = deployRouterMock(IVault(address(vault)), weth, permit2);
vm.label(address(router), "router");
batchRouter = deployBatchRouterMock(IVault(address(vault)), weth, permit2);
vm.label(address(batchRouter), "batch router");
feeController = vault.getProtocolFeeController();
vm.label(address(feeController), "fee controller");
vm.startPrank(address(vaultAdmin));
updateWeightRunner = new MockUpdateWeightRunner(address(vaultAdmin), address(addr2), true);
vm.label(address(updateWeightRunner), "updateWeightRunner");
updateWeightRunner.setQuantAMMSwapFeeTake(0);
vm.stopPrank();
vm.startPrank(owner);
upliftOnlyRouter = new UpliftOnlyExample(
IVault(address(vault)),
weth,
permit2,
@> 0,
@> 0,
@> address(0),
@> "",
"Uplift LiquidityPosition v1",
"Uplift LiquidityPosition v1"
);
vm.stopPrank();
vm.label(address(upliftOnlyRouter), "upliftOnlyRouter");
poolHooksContract = address(upliftOnlyRouter);
(pool,) = createPool();
for (uint256 i = 0; i < users.length; ++i) {
address user = users[i];
vm.startPrank(user);
approveForSender();
vm.stopPrank();
}
if (pool != address(0)) {
approveForPool(IERC20(pool));
}
initPool();
(daiIdx, usdcIdx) = getSortedIndexes(address(dai), address(usdc));
}
@> is used to show the changes in the code snippet.
The _updateWeightRunnerParam parameter has no gettter in the contract, so we performed some mutational testing to check the contract's behavior with invalid parameter. Go to UpliftOnlyExample.sol and make the _updateWeightRunner variable public.
address public immutable _updateWeightRunner;
Now add the following test snippet just below the setUp function in the UpliftExample.t.sol file:
function testUpliftExampleConstructorValues() public {
console.log("version: ", upliftOnlyRouter.version());
console.log("upliftFeeBps: ", upliftOnlyRouter.upliftFeeBps());
console.log("minWithdrawalFeeBps: ", upliftOnlyRouter.minWithdrawalFeeBps());
console.log("updateWeightRunner: ", upliftOnlyRouter._updateWeightRunner());
assertEq(bytes(upliftOnlyRouter.version()).length, 0, "version should be empty");
assertEq(upliftOnlyRouter.upliftFeeBps(), 0, "upliftFeeBps should be 0");
assertEq(upliftOnlyRouter.minWithdrawalFeeBps(), 0, "minWithdrawalFeeBps should be 0");
assertEq(upliftOnlyRouter._updateWeightRunner(), address(0), "updateWeightRunner should be 0");
}
Now run the test cases using the following command: (Make sure you are in the pool-hooks directory)
forge test --mt testUpliftExampleConstructorValues -vv
As we can see in the output, the contract is deployed with invalid parameters and the contract's intended functionality is disrupted.
[⠢] Compiling...
No files changed, compilation skipped
Ran 1 test for test/foundry/UpliftExample.t.sol:UpliftOnlyExampleTest
[PASS] testUpliftExampleConstructorValues() (gas: 28500)
Logs:
version:
upliftFeeBps: 0
minWithdrawalFeeBps: 0
updateWeightRunner: 0x0000000000000000000000000000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 25.40ms (244.00µs CPU time)
Ran 1 test suite in 27.21ms (25.40ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
The contract could be deployed with invalid parameters.
The contract's intended functionality could be disrupted.
An empty string value for version could be a potential issue.
The parameters _upliftFeeBps, and _minWithdrawalFeeBps are used to evaluate fee per LP (Liquidity Provider) and if these values are set to zero or to some other undesired hostile value, it could disrupt the contract's intended functionality.
Same for _updateWeightRunner which is used to update the weight of the pool.
Fee per LP (Liquidity Provider) could be set to zero or to some other undesired hostile value.
Tools Used
Foundry (Framework)
Chisel
Forge test
Mutational Testing
Recommendation
Add a modifier to check the validity of the version parameter.
Add value validation for _upliftFeeBps and _minWithdrawalFeeBps parameters.
Add address validation for _updateWeightRunner parameter.
One possible solution could be the following:
+ uint256 constant MAX_FEE_BPS = 10000; // 100%
+ uint256 constant MIN_FEE_BPS = 10; // 10%
+ uint256 constant MAX_WITHDRAWAL_FEE_BPS = 10000; // 100%
+ uint256 constant MIN_WITHDRAWAL_FEE_BPS = 10; // 10%
+ modifier validVersion(string memory _version) {
+ require(bytes(_version).length > 0, "VERSIONREQ");
+ _;
+ }
constructor(
IVault vault,
IWETH weth,
IPermit2 permit2,
uint64 _upliftFeeBps,
uint64 _minWithdrawalFeeBps,
address _updateWeightRunnerParam,
string memory version,
string memory name,
string memory symbol
- ) MinimalRouter(vault, weth, permit2, version) Ownable(msg.sender) {
+ ) validVersion(version) MinimalRouter(vault, weth, permit2, version) Ownable(msg.sender) {
require(bytes(name).length > 0 && bytes(symbol).length > 0, "NAMEREQ"); //Must provide a name / symbol
+ require(_updateWeightRunnerParam != address(0), "INVALID_UPDATE_WEIGHT_RUNNER");
+ require(_upliftFeeBps >= MIN_FEE_BPS && _upliftFeeBps <= MAX_FEE_BPS, "INVALID_FEE_BPS");
+ require(_minWithdrawalFeeBps >= MIN_WITHDRAWAL_FEE_BPS && _minWithdrawalFeeBps <= MAX_WITHDRAWAL_FEE_BPS, "INVALID_WITHDRAWAL_FEE_BPS");
lpNFT = new LPNFT(name, symbol, address(this));
upliftFeeBps = _upliftFeeBps;
minWithdrawalFeeBps = _minWithdrawalFeeBps;
_updateWeightRunner = _updateWeightRunnerParam;
}
The above solution adds a modifier validVersion to check the validity of the version parameter. It also adds value validation for _upliftFeeBps and _minWithdrawalFeeBps parameters. Moreover, it adds address validation for _updateWeightRunner parameter. Therefore, enhancing and reducing the security and common pitfalls of the contract.
