AQTIS Liquid Staking Tokens

The protocol doesn’t provide on-chain guarantees that rewards will be available for a set amount of time, and neither withdrawals other than the promise that the pool will be watched and kept at a reasonable state (the component responsible for this is the Ecosystem Liquidity aggregator, which is not part of this audit). In the case of a liquidation event the last users to exit will very likely lose funds due to the nature of uniswap pools.

It is unclear at this point in time what mechanism the team will be utilizing to perform these actions daily, however it is important that these mechanisms are reliable and predictable as if these actions aren’t performed, users will lose out on rewards (Although they might be negligible if only a single day is missed) and users might have to deal with a stale price of AQTIS.

On top of the typical ERC20 balance accounting, LST transfer and minting operations involve some protocol-introduced reward accounting. This is implemented in AbstractRewards::_updateRecord which is invoked inside each LST’s overridden ERC20::_update function:

function _updateRecord(address user, uint256 value, Update updateType) internal {
	_beforeUpdate(user, value, updateType);
	_updateSupply(user, value, updateType);

	if (user == address(0) || isContract(user)) {
    	return;
	}

	// If user is not in the list, add them
	if (!_users.contains(user)) {
    	_users.add(user);
	}

	UserRecord storage record = _userRecords[user];
	uint256 timeElapsed = 0;

	// First entry check
	if (record.lastUpdateTime == 0) {
    	record.cumulativeBalance = 0;
    	record.cumCirculatingSupplyLastClaim = _supply.cumulativeCirculatingSupply;
    	record.lastClaimTime = block.timestamp;
	} else {
    	timeElapsed = block.timestamp - record.lastUpdateTime;
    	record.cumulativeBalance += record.userBalance * timeElapsed;
	}

	// Update balance and last update time
	if (updateType == Update.FROM) {
    	record.userBalance -= value;
	} else {
    	record.userBalance += value;
	}
	record.lastUpdateTime = block.timestamp;
}

The protocol skips all reward accounting for users that are considered to be smart contracts ( i.e., addresses for which isContract returns true )

function isContract(address addr) internal view returns (bool) {
    	if (whitelistedContracts[addr]) {
        	return false;
    	}
    	uint size;
    	assembly {
        	size := extcodesize(addr)
    	}
    	return size > 0;
}

Although this is done with the clear intent of preventing liquidity pools (or other non-whitelisted 3rd party contracts) from accruing rewards , isContract(X) is false when:

• X has not been deployed, this means that interacting with an about-to-be deployed address will cause rewarding accounting to take place as normal.
• X is under construction, this enables one to run code from inside a contract’s constructor and having reward accounting enabled for that contract momentarily.

One can use the first point from above to set the reward accounting of about-to-be deployed contract arbitrarily high:

• Assume that a contract X receives a huge LST flashloan of N tokens from the corresponding Balancer/Uniswap pool
• X transfers all borrowed funds to an address Y that will be soon deployed at a pre-computed address via CREATE2. At this point, AbstractRewards::_updateRecord will see that isContract(Y) is false, so it will happily create a _userRecord for Y during the large transfer of tokens
• Y is deployed via CREATE2 to the predicted address
• Assume that Y has a function that simply transfers all LST funds back to X, named Y::refund() . Here's where everything comes together; when we invoke Y::refund() the transfer method of the LST will not touch the _userRecord entry of Y, since Y is now initialized and isContract(Y) will return true. Token balances will be updated as they should, so X will receive back the funds and repay the flash loan with no problems.

We have now successfully polluted the _userRecord of Y with a completely inflated balance ( even though Y holds no tokens ). This can have devastating consequences: AbstractRewards::_getTWAB(Y) will return N ( the originally flash-loaned amount ) when invoked at a subsequent block than the one that Y got created.

At this point, we can use this to directly attack ClaimVault and claim rewards as if Y had N tokens, since rewards will be claimed successfully. The above attack can be atomically set up with many Y-like contracts, and thus we can claim rewards multiple times from the ClaimVault contract and drain its entire balance.

Flash Loans are not necessary, an LST holder can also pull the attack on its own via direct transfers. However, flash loans make the attack even more practical, since no LST balance is required at all to pull this out.

We additionally note another vector of reward accounting manipulation, although with a lower severity than the manipulation described above. This time we are targeting the accounting inside AbstractRewards::_updateSupply:

AbstractRewards::_updateSupply:78

if (updateType == Update.FROM && (user == address(0) || isContract(user))) {
        	_supply.currentCirculatingSupply += value;
    	} else if (updateType == Update.TO && (user == address(0) || isContract(user))) {
        	_supply.currentCirculatingSupply -= value;
}
• Assume that an attacker transfers N LST tokens to a pre-calculated address and then deploys a contract X on it (_supply.currentCirculatingSupply has not changed since isContract(X) was false when the transfer took place)
• Assume that X transfers the LST tokens at another pre-calculated address. Since isContract(X) is now true, _supply.currentCirculatingSupply is now increased by N .
• After the transfer, X uses CREATE2 to deploy a copy of itself at the pre-computed address Y of the previous step
• Step 2 is repeated many times

This artificially inflates the _supply.currentCirculatingSupply. X may be coded in a way so that funds are returned to the attacker after a finite number of steps so the attacker ultimately manipulates the reward accounting for free. This manipulation is not by itself profitable for the attacker, but it could cause QrtReward::getRewardsFor and MintingBonus::_claimableBonusRewards to yield lower rewards for the rest of the protocol users.

In conclusion, even though the design decision of disabling reward distribution for smart contracts is a fine decision on its own, it currently poses a security liability. It is recommended that this part of the protocol is re-implemented and/or re-designed.

If maxClaim is larger than remainingRewards in MintingBonus, claimVault::_claim will keep reverting till the bonus program finishes, since remainingRewards will underflow and revert due to Solidity’s 0.8 integration of safeMath resulting in a DoS.

// @audit this should be the other way round cause if remaining < maxClaim, then it fails
if (bonusAmount > rewardsSettings[lst].maxClaim) {
     bonusAmount = rewardsSettings[lst].maxClaim;
} else if (bonusAmount > rewardsSettings[lst].remainingRewards) {
     bonusAmount = rewardsSettings[lst].remainingRewards;
}

In order to prevent the protocol from consuming stale prices in the event of a price feed’s downtime, answers coming from chainlink data feeds should be checked for staleness. Reference

/// @notice Returns ETH Price in USD (8 decimals)
function getLatestEthPrice() public view returns (int) {
    (,int price,,,) = priceFeedEth.latestRoundData();
    return price;
}

// @audit Returns USDC price in USD (8 decimals) *FIX
/// @notice Returns USD Price in USD (8 decimals)
function getLatestUsdPrice() public view returns (int) {
    (,int price,,,) = priceFeedUsd.latestRoundData();
    return price;
}

The protocol has some centralization risks, with some owner entities considered trusted. There are numerous instances in which the owner of a contract is able to change the core configuration of the contracts (e.g., ClaimVault::setUsdcPair ) or even pull funds out of the contract ( e.g., ClaimVault::withdrawERC20 )

Although AbstractLST emits a wealth of events, there are no events emitted for any changes in the claimVault or any bonus rewards that are set. The events should be reconsidered protocol wide.

Although written to, _users is never read from in the protocol, and it isn’t exposed publicly either since it’s internal.

EnumerableSet.AddressSet internal _users; // @audit this variable isn't read anywhere and not exposed publicly.

The check is redundant since if timeDifference == 0,
(record.cumulativeBalance + (record.userBalance * timeDifference)) / claimTime

==
(record.cumulativeBalance + (record.userBalance * 0)) / claimTime
==
record.cumulativeBalance / claimTime

if (timeDifference > 0) {
    return (record.cumulativeBalance + (record.userBalance * timeDifference)) / claimTime;
} else {
    return record.cumulativeBalance / claimTime;
}

This would result in certain users/all users subscribed to the batch daily claim to potentially getting their rewards a day late. Suggested fix is to lower this to half a day.

// @audit is there actually a reason for the claimCooldown to be 1 day? Since this could potentially end up reverting for the
// batch claimer, if it deviates a bit from the exact 1 day mark.
require(lastClaimTime[user] + claimCooldown < block.timestamp, "ClaimVault: Cooldown not expired");

Although the project already has quite good coverage of tests it does not achieve 100% test coverage. It is recommended that before launching a further investment of testing aiming to cover BatchClaim, ClaimVault and MintingBonus especially is done. Fuzz tests are also recommended as they could catch further edge case logic issues.

The amountOutMin member of the LSTSwap::SwapCallbackData is not used anywhere

struct SwapCallbackData {
    	address tokenIn;
    	address tokenOut;
    	uint256 amountOutMin;
    	uint256 amountInMax;
}

Since anybody may enable “auto-claiming” of rewards, BatchClaim::BatchClaim might cause the execution to run out of gas when _enabledUsers become large enough

function batchClaim() external onlyScheduler {
    	address[] memory lsts = claimVault.getLSTs();

    	for (uint256 i = 0; i < _enabledUsers.length(); i++) {
        	address user = _enabledUsers.at(i);
        	for (uint256 j = 0; j < lsts.length; j++) {
            	claimRewardsFor(lsts[j], user);
        	}
    	}
}

This might not pose a practical security issue, since the scheduler will be able to claim rewards for a subset of users at a time by calling BatchClaim::multiClaim or BatchClaim::multiClaimLST.

Both LSTSwap::_setTimeWeightedAveragePeriod and LSTSwap::_setMinOutFractionQ64 are not used anywhere inside the codebase

function _setTimeWeightedAveragePeriod(uint24 period) internal {
	timeWeightedAveragePeriod = period;
}

function _setMinOutFractionQ64(uint256 fraction) internal {
	minOutFractionQ64 = fraction;
}

Compose::minAmountBuy can never be set to 0:

function distributeFunds(
	uint256 _percentageQeth,
	uint256 _percentageQsd,
	uint256 _percentageQrt
) external payable nonReentrant {
	require(msg.value > 0, "No ETH sent");
	...
	require(
        	msg.value >= minAmountBuy,
        	"Amount sent is less than minimum amount"
    	);

So the first check involving msg.value inside Compose::distributeFunds is unnecessary, since the one that follows is logically stronger:

function distributeFunds(
	uint256 _percentageQeth,
	uint256 _percentageQsd,
	uint256 _percentageQrt
) external payable nonReentrant {
	require(msg.value > 0, "No ETH sent");
	...
	require(
        	msg.value >= minAmountBuy,
        	"Amount sent is less than minimum amount"
    	);

asd

The payer argument of LSTSwap::pay is unused:

function pay(address token, address payer, address receiver, uint256 amount) private {
	ILST(token).mint(amount);
	TransferHelper.safeTransfer(token, receiver, amount);
}

The code is compiled with Solidity 0.8.23, which has no known issues at the time of this report.