ProfitManager smart contract, there are a few functions where front-running

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L195
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L292
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L439

Vulnerability details

Impact

Consider these security enhancements into the ProfitManager contract, there are a few functions where front-running could potentially be a concern. However, due to the nature of the functions and the contract's design, the risk may vary. Let's identify these functions and discuss potential mitigation strategies:

Potential Targets for Front-Running

1. notifyPnL Function:
   This function updates the system's profit and loss (PnL) information. If the function's effects significantly impact the contract's state (like updating creditMultiplier or surplusBuffer), it could be a target for front-running, especially if these updates influence subsequent transactions.

2. claimRewards Function:
   This function allows users to claim their rewards. If the rewards are significant and based on changing factors like gauge weights or profit indices, there might be an incentive for users to front-run others to claim rewards under more favorable conditions.

3. setProfitSharingConfig:
   Any function that can alter the contract's critical parameters (like profit sharing percentages) could be targeted. Front-runners might aim to preemptively act before a new configuration takes effect.

Proof of Concept

Let's consider a hypothetical front-running attack scenario involving the notifyPnL and claimRewards functions. The scenario will demonstrate how an attacker could potentially exploit the timing of these function calls for financial gain.

Scenario Setup:
notifyPnL Function:
This function updates the profit and loss information for a given gauge, which can affect the distribution of profits and potentially the value of rewards for GUILD token holders.

claimRewards Function:
This function allows users to claim their share of profits based on their participation in different gauges.

Hypothetical Front-Running Attack:
Step 1: Observation
An attacker monitors the Ethereum mempool and observes a pending transaction that calls notifyPnL, indicating a significant profit for a specific gauge. This profit, once distributed, will substantially increase the rewards for users involved in that gauge.

Step 2: Front-Running Transaction
Before the notifyPnL transaction is processed, the attacker quickly sends a transaction with a higher gas price to call claimRewards for themselves. The attacker aims to claim rewards at the current gauge profit index, which is about to increase significantly due to the pending notifyPnL transaction.

Step 3: notifyPnL Transaction
The notifyPnL transaction is processed after the attacker's claimRewards transaction. This updates the gauge profit index, increasing the value of rewards for the gauge.

Step 4: Regular User's Transaction
A regular user who was also entitled to claim rewards from the same gauge executes their claimRewards transaction. However, since the attacker already claimed rewards at the more favorable conditions, the regular user receives a smaller share of the profits than they would have if they had claimed before the notifyPnL update.

Step 5: Exploitation
The attacker takes advantage of the timing to extract more value from the profit distribution mechanism, reducing the potential rewards for other participants.

For Preventing Front-Running Attacks
Here's how the modified contract would look:

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.13;

import {CoreRef} from "@src/core/CoreRef.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {SimplePSM} from "@src/loan/SimplePSM.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";

contract ProfitManager is CoreRef {
// ... [Other variables and struct declarations]

bool private _locked; // Reentrancy guard state variable

modifier noReentrant() {
    require(!_locked, "Reentrant call");
    _locked = true;
    _;
    _locked = false;
}

// ... [Constructor and events]

// ... [Functions like minBorrow, initializeReferences, etc.]

// Modified notifyPnL function with reentrancy guard
function notifyPnL(address gauge, int256 amount) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) noReentrant {
    // Existing function logic...
}

// Modified claimGaugeRewards function with reentrancy guard
function claimGaugeRewards(address user, address gauge) public noReentrant returns (uint256 creditEarned) {
    // Existing function logic...
}

// Optimized claimRewards function to allow specific gauges
function claimRewards(address user, address[] calldata specificGauges) external returns (uint256 creditEarned) {
    for (uint256 i = 0; i < specificGauges.length; ++i) {
        creditEarned += claimGaugeRewards(user, specificGauges[i]);
    }
}

To implement checks against potential front-running in the notifyPnL and setProfitSharingConfig functions of your ProfitManager contract, you can adopt the following strategies:

1. Implementing Time Delays for setProfitSharingConfig
   Introduce a delay mechanism for changes in critical parameters like profit sharing percentages. This can be achieved using a two-step process: propose a change and then execute it after a certain period, ensuring transparency and reducing the chance of front-running.
   Code Implementation:
   First, add a structure to store proposed changes and a delay constant:

struct ProfitSharingProposal {
uint256 surplusBufferSplit;
uint256 creditSplit;
uint256 guildSplit;
uint256 otherSplit;
address otherRecipient;
uint256 proposedAtBlock;
}

mapping(bytes32 => ProfitSharingProposal) public profitSharingProposals;
uint256 public constant PROPOSAL_DELAY = 256; // Number of blocks before a proposal can be executed

Modify the setProfitSharingConfig function to propose changes:

function proposeProfitSharingConfig(
uint256 surplusBufferSplit,
uint256 creditSplit,
uint256 guildSplit,
uint256 otherSplit,
address otherRecipient
) external onlyCoreRole(CoreRoles.GOVERNOR) {
bytes32 proposalId = keccak256(abi.encodePacked(surplusBufferSplit, creditSplit, guildSplit, otherSplit, otherRecipient, block.number));
profitSharingProposals[proposalId] = ProfitSharingProposal({
surplusBufferSplit: surplusBufferSplit,
creditSplit: creditSplit,
guildSplit: guildSplit,
otherSplit: otherSplit,
otherRecipient: otherRecipient,
proposedAtBlock: block.number
});
// Emit an event for the proposal
}

function executeProfitSharingConfig(bytes32 proposalId) external onlyCoreRole(CoreRoles.GOVERNOR) {
ProfitSharingProposal storage proposal = profitSharingProposals[proposalId];
require(block.number >= proposal.proposedAtBlock + PROPOSAL_DELAY, "Proposal delay not met");
// Set the new profit sharing config here
// Emit an event for execution
}

2. Preventing Front-Running in notifyPnL
   This function could be targeted for front-running due to its potential impact on creditMultiplier or surplusBuffer. To mitigate this, consider the following:
   Randomize Execution:
   Introduce randomness in how transactions are processed to make front-running less predictable.
   Transaction Throttling:
   Limit the frequency at which this function can be called to prevent rapid, opportunistic changes.

Code Implementation for Randomization/Throttling:

uint256 public lastPnLUpdateBlock;

function notifyPnL(address gauge, int256 amount) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
require(block.number > lastPnLUpdateBlock + PNL_UPDATE_DELAY, "Update delay not met");
lastPnLUpdateBlock = block.number;
// Existing logic for notifyPnL...
}

Tools Used

VS Code

Recommended Mitigation Steps

Key Modifications:

1. Reentrancy Guard:
   A state variable _locked and a noReentrant modifier are added. This modifier is applied to functions that make external calls and could be vulnerable to reentrancy attacks, like notifyPnL and claimGaugeRewards.

bool private _locked;

modifier noReentrant() {
require(!_locked, "Reentrant call");
_locked = true;
_;
_locked = false;
}
Apply this modifier to functions that make external calls, e.g., notifyPnL, claimGaugeRewards, etc.

2. Optimized claimRewards for gas:
   The claimRewards function is modified to accept an array of specific gauges. This reduces gas costs by allowing users to claim rewards for only the gauges they're interested in.
   To address the identified concerns of reentrancy attacks, gas optimization, and front-running/time manipulation risks in your ProfitManager contract, let's implement specific checks and modifications:
   Reentrancy Attack Prevention
   Checks-Effects-Interactions Pattern: Modify functions that make external calls to follow this pattern. For example, in functions like notifyPnL, the state changes should be made before the external calls to CreditToken(credit).burn() or CreditToken(credit).transfer().

Reentrancy Guard: Utilize a reentrancy guard modifier. This can be done by adding a state variable, typically named something like _locked, which is checked and updated in the modifier.

Gas Optimization
Optimize claimRewards Function: Instead of iterating through all gauges for each user, consider implementing a mechanism where users claim rewards for specific gauges they're interested in. This reduces the gas cost significantly, especially when the number of gauges is large.

function claimRewards(address user, address[] calldata specificGauges) external returns (uint256 creditEarned) {
for (uint256 i = 0; i < specificGauges.length; ++i) {
creditEarned += claimGaugeRewards(user, specificGauges[i]);
}
}
Reduce State Writes: Minimize the number of state changes within loops. For instance, in claimGaugeRewards, if you can accumulate changes in memory and write them to storage at the end, it could save gas.

These measures enhance the security of the contract against front-running but also add complexity.

1. Time Delays for setProfitSharingConfig:
   Changes Made:Introduced a two-step process for updating the profit sharing configuration.
   Added a ProfitSharingProposal struct to store proposed changes.
   Created a mapping profitSharingProposals to track these proposals.
   Implemented a proposeProfitSharingConfig function to initiate a change.
   Added an executeProfitSharingConfig function to apply the change after a delay.
   Why:To prevent sudden changes in critical parameters, which could be exploited by front-runners.
   The delay between proposal and execution ensures transparency and gives stakeholders time to react, reducing the likelihood of opportunistic front-running.

2. Randomization/Throttling in notifyPnL
   Changes Made:Implemented a check to ensure a minimum number of blocks have passed before this function can be called again.
   Added a lastPnLUpdateBlock state variable to track the last update.
   Why:To mitigate the risk of front-running in functions that can significantly impact the contract's state, like updating the creditMultiplier or surplusBuffer.
   Throttling the frequency of updates makes it harder for front-runners to time their transactions based on predictable state changes.
   Overall Security Enhancement
   These changes collectively aim to enhance the contract's resilience against front-running attacks by:

3. Increasing Predictability:
   By delaying critical parameter changes, users are given more visibility and predictability, reducing the potential for sudden advantageous moves by front-runners.
   Reducing Exploitability: By throttling the frequency of critical updates, it becomes more challenging for attackers to execute front-running strategies based on immediate state changes.

Conclusion
These modifications are crucial for maintaining the integrity of the ProfitManager contract, especially considering its role in managing financial transactions and profit distribution. They help ensure that changes within the contract are conducted in a controlled and transparent manner, safeguarding against manipulation and potential exploits.

Assessed type

Access Control

[0xSorryNotSorry]

no sender

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[thebrittfactor]

Closing due to warden data missing. Appears to be a duplicate of issue #52.