6.1 Blocklist bypass via increaseAllowance and transferFrom
The UWattsVault4626 contract implements a blocklist mechanism by overriding the approve, transfer, and transferFrom functions with the onlyNotBlocked modifier. This modifier checks that msg.sender is not in the isBlocked mapping.
However, the contract inherits from OZ's ERC20Upgradeable version 4.9.6 but does not override increaseAllowance or decreaseAllowance. As a result, a blocked user can still modify allowances by calling increaseAllowance (which calls the internal _approve function and bypasses the overridden approve check).
Furthermore, the transferFrom override only enforces the onlyNotBlocked check on msg.sender (the spender). It does not validate the status of the from or to addresses.
function transferFrom(
address from,
address to,
uint256 value
)
public
override(ERC20Upgradeable, IERC20Upgradeable)
// @audit-issue Checks msg.sender (spender), but allows transfer if 'from'
// is blocked.
onlyNotBlocked
returns (bool)
{
return super.transferFrom(from, to, value);
}
This creates two bypass vectors:
- Bypass via new allowance: A blocked user calls
increaseAllowance to authorize a fresh, unblocked wallet. The unblocked wallet then calls transferFrom to move the funds. Since the non-blocked spender is msg.sender in transferFrom, the check passes; - Bypass via existing allowance: If a user provided an allowance before being blocked, the authorized spender can still move funds because the owner's blocked status is ignored during
transferFrom;
Recommendation
Consider overriding the _beforeTokenTransfer hook to enforce blocklist checks. Alternatively, modify the internal _transfer and _approve functions to check that the from and to are not blocklisted.
Resolution
Replaced per-function onlyNotBlocked modifier overrides on approve, transfer, and transferFrom with a single _beforeTokenTransfer hook that validates from, to, and msg.sender against the blocklist. This closes the bypass vectors through increaseAllowance/ decreaseAllowance and ensures blocked addresses cannot operate as spenders via pre-existing allowances.
6.2 Sequential fee application in harvest(...) results in lower than expected harvester bounties
The harvest(...) function in the StakingStrategy contract is responsible for claiming rewards, deducting fees, and auto-compounding the remaining tokens back into the staking protocol. To ensure the system remains sustainable and incentivized, the protocol defines a performanceFeeBps for the treasury and a harvesterBountyBps to reward the caller who triggers the harvest.
According to the setFees(...) function, the total fees are capped at 2000 basis points (20%) of the total rewards, calculated as the sum of both fee components:
function setFees(
uint256 _performanceFeeBps,
uint256 _harvesterBountyBps
) external onlyOwner {
// @audit Total allowed fees are validated as the sum of both BPS values
if (_performanceFeeBps + _harvesterBountyBps > 2000)
revert FeesTooHigh(); // max 20% total
// ...
}
However, the harvest(...) function applies these fees sequentially rather than in parallel against the original reward balance. Specifically, the performance fee is subtracted first, and the harvester bounty is then calculated based on the _remaining_ balance.
// ...
// 2. Deduct performance fee
uint256 perfFee = (rewardBal * performanceFeeBps) / BPS;
if (perfFee > 0) {
rewardToken.safeTransfer(feeRecipient, perfFee);
}
// @audit The performance fee is subtracted from the total reward balance
uint256 remaining = rewardBal - perfFee;
// 3. Pay harvester bounty (incentivizes public harvest calls)
// @audit-issue The bounty is calculated on the 'remaining' amount instead of 'rewardBal'
uint256 bounty = (remaining * harvesterBountyBps) / BPS;
if (bounty > 0) {
rewardToken.safeTransfer(caller, bounty);
remaining -= bounty;
}
// ...
This logic means the actual bounty paid to the harvester is (Reward * (1 - performanceFee) * harvesterBounty) instead of the expected (Reward * harvesterBounty). Consequently, the harvester receives less than the percentage defined in harvesterBountyBps, which may lead to insufficient incentives for external actors to maintain the vault's compounding frequency, especially when the performance fee is high.
Recommendation
Consider calculating both the perfFee and the bounty based on the initial rewardBal to ensure the distributed amounts match the basis points configured in the contract state.
Resolution
Both perfFee and bounty are now calculated in parallel using the original rewardBal as the reference value. This ensures that the distributed amounts exactly match the basis points configured through setFees().
6.3 Strategy replacement can lead to loss of funds
The UWattsVault4626 contract delegates asset management to the StakingStrategy contract. The owner can update this strategy reference using the setStrategy(...) function.
However, the setStrategy function only updates the strategy state variable pointer to the new address without withdrawing the underlying assets from the previous strategy or migrating them to the new one.
function setStrategy(address _strategy) external onlyOwner {
if (_strategy == address(0)) revert ZeroAddress();
IStrategy newStrategy = IStrategy(_strategy);
// Verify strategy asset matches vault asset
if (newStrategy.asset() != asset()) revert StrategyAssetMismatch();
address oldStrategy = address(strategy);
// @audit-issue The pointer is updated, but funds remain in 'oldStrategy'.
strategy = newStrategy;
emit StrategyUpdated(oldStrategy, _strategy);
}
Once the strategy variable is overwritten, the UWattsVault4626 loses the ability to interact with the old strategy.
- The
UWattsVault4626 can only call withdraw on the address stored in the strategy variable; - The
StakingStrategy restricts the withdraw function to onlyVault; - The
StakingStrategy also explicitly forbids the owner from using recoverToken to retrieve the underlying asset;
This leads to a situation where funds held by the old strategy become locked. Furthermore, the UWattsVault4626's totalAssets() function immediately queries the _new_ strategy (which likely has zero balance), causing the share price to crash to zero.
function totalAssets() public view override returns (uint256) {
if (address(strategy) == address(0)) {
return 0;
}
// @audit Queries the NEW empty strategy, ignoring funds in the old one.
return strategy.totalUnderlying();
}
Recommendation
Consider the above consequences when deciding to update the strategy contract. Before updating the strategy variable, the vault should withdraw the assets from the current strategy, approve the new strategy, and deposit the assets into it.
Resolution
Future logic upgrades can be performed through proxy upgrades to the strategy implementation itself. Therefore, a post initialization setStrategy() function is unnecessary.
The fix removes the public setStrategy() function entirely and replaces it with an internal _setStrategy() function that is executed exactly once during initialization. The strategy address is now immutable after deployment.
6.4 The harvest(...) function logic may be inconsistent depending on the Quoter implementation
The harvest(...) function in the StakingStrategy contract is designed to compound rewards by choosing the most cost-effective source for uWatt tokens between a Decentralized Exchange (DEX) and a designated uWattVendor. To make this decision, it utilizes a quoter to estimate the output from the DEX.
function harvest(
address caller
) external override nonReentrant returns (uint256 uWattGained) {
// ...
if (remaining > 0) {
// @audit The quoter is used here to compare prices
uint256 uWattsFromDEX = quoter.quoteExactInput(
rewardToAssetPath,
remaining
);
uint256 uWattsFromVendor = uWattVendor.getUWattAmount(
remaining,
address(this)
);
if (uWattsFromDEX > uWattsFromVendor) {
// DEX is cheaper
uWattSource = "DEX";
// @audit This internal call calls the quoter again to set slippage
uint256 minUWattOut = _getMinimumSwapOutput(remaining);
// ...
IRouter.ExactInputParams memory params = IRouter
.ExactInputParams({
path: rewardToAssetPath,
recipient: address(this),
amountIn: remaining,
amountOutMinimum: minUWattOut
});
uWattGained = router.exactInput(params);
} else {
// ...
}
}
// ...
}
The logic assumes a specific behavior from the IQuoter implementation. Depending on whether the quoter simulates a full swap or returns a spot price, two issues arise:
- Simulation-based Quoter: If the quoter (e.g., Uniswap V3
QuoterV2) simulates a full swap, it returns the exact amountOut expected (excluding slippage from state changes between the quote and execution). In this case, calculating minUWattOut by applying a maxSlippageBps reduction to the already accurate quote might be redundant or incorrectly calibrated, as uWattsFromDEX is already the "best estimate."; - Spot-price-based Quoter: If the quoter returns a value based on the current spot price without simulating the impact of the swap size (price impact), the comparison
if (uWattsFromDEX > uWattsFromVendor) becomes inaccurate. The actual amount received from the DEX after price impact could be significantly lower than uWattsFromVendor, leading the strategy to pick the DEX when the Vendor was actually the cheaper option;
Recommendation
Consider re-evaluating the integration with the quoter to ensure it aligns with the specific implementation used (e.g., Uniswap V3 QuoterV2).
Resolution
We agree that the contract previously did not clearly communicate which type of quoter implementation is required. The harvest() comparison between DEX and Vendor output is only valid when backed by a simulation-based quoter (e.g., Uniswap V3 `QuoterV2`) that accounts for full price impact. A spot-price quoter would overestimate DEX output and cause the strategy to prefer the DEX even when the Vendor yields more uWatts. To prevent future misconfiguration, explicit documentation has been added.
6.5 The reportRewards function is incompatible with tokens that revert on zero approvals
The reportRewards(...) function assumes that all ERC20 tokens will successfully execute an approve(...) call when the amount is set to 0. There is a known category of non-standard ERC20 tokens (weird tokens), such as the older BNB token on Ethereum ( 0xB8c77482e45F1F44dE1745F52C74426C631bDD52), that revert when their approve(...) function is called with a 0 value. This category of ERC20s can not be used as staking rewards
Recommendation
For this category of tokens, the allowance should be set directly to the new value.
Resolution
We've replaced it with forceApprove, which handles edge cases.
6.6 Unused code
In the SuperfluidAdapter contract, the following errors are unused:
error InvalidGDAForwarder();
error UnitsAlreadySynced();
error PoolNotConfigured();
As well as the _downgradeTo function:
function _downgradeTo(address _to, uint256 _amount) internal {
if (_to == address(0)) revert ZeroAddressNotAllowed();
superfluidToken.downgradeTo(_to, _amount);
}
Also, the treasury state variable inside the StakingUWatts seems to be unused. It is only set during initialization, but no function calls or uses the treasury.
Recommendation
Consider removing the dead code to improve readability.
Resolution
The code has been removed.
6.7 Redundant calls to the quoter contract in harvest(...) increase gas consumption
The harvest(...) function in the StakingStrategy contract is designed to collect rewards, determine the most cost-effective source (DEX vs. Vendor) to acquire assetToken, and compound those rewards. When evaluating the DEX as a potential source, the contract utilizes a quoter contract (Uniswap V3 QuoterV2) to simulate the swap and determine the expected output.
Currently, if the DEX is identified as the better option, the contract calls _getMinimumSwapOutput(...) to calculate the slippage protection. This internal function performs a second, identical call to quoter.quoteExactInput(...) with the same parameters already used in the main harvest(...) logic.
As noted in the Uniswap documentation, Quoter contracts are intended primarily for off-chain estimation because they perform a full state-simulated swap, which is computationally expensive and gas-intensive when executed on-chain. Calling this function twice in a single transaction significantly increases the gas cost for the harvester.
function harvest(...) external override nonReentrant returns (uint256 uWattGained) {
// ...
if (remaining > 0) {
// @audit First call to the quoter to compare sources
uint256 uWattsFromDEX = quoter.quoteExactInput(
rewardToAssetPath,
remaining
);
uint256 uWattsFromVendor = uWattVendor.getUWattAmount(
remaining,
address(this)
);
if (uWattsFromDEX > uWattsFromVendor) {
uWattSource = "DEX";
// @audit Second call to the quoter inside this function
uint256 minUWattOut = _getMinimumSwapOutput(remaining);
// ...
}
}
// ...
}
The redundant call occurs here:
function _getMinimumSwapOutput(
uint256 rewardAmount
) internal returns (uint256) {
// @audit-issue Redundant quoter call; the value was already fetched in harvest()
uint256 expectedUWattOut = quoter.quoteExactInput(
rewardToAssetPath,
rewardAmount
);
return (expectedUWattOut * (BPS - maxSlippageBps)) / BPS;
}
Recommendation
Consider refactoring the _getMinimumSwapOutput(...) function to accept the expectedUWattOut as an input parameter instead of fetching it again. This allows the harvest(...) function to reuse the result from the first quoter call, significantly reducing the gas overhead of the compounding process.
Resolution
The redundant second call to quoter.quoteExactInput inside _getMinimumSwapOutput has been removed. The previously obtained uWattsFromDEX value is now reused directly to calculate the slippage floor, eliminating both the inconsistency risk between quotes and the unnecessary gas overhead.
