Skip to content

Audit cyfrin #155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Audit cyfrin #155

wants to merge 25 commits into from

Conversation

gonzaloetjo
Copy link
Collaborator

@gonzaloetjo gonzaloetjo commented May 28, 2025

Linked issues

Solved

On-going:

Aknowledged

Changes

Modified tests o work correctly (some may be missing in this list):

  • H09
  • H10
  • M09

@gonzaloetjo gonzaloetjo force-pushed the audit-cyfrin branch 2 times, most recently from 63674d6 to ce10fda Compare June 10, 2025 10:09
@gonzaloetjo gonzaloetjo force-pushed the audit-cyfrin branch 2 times, most recently from 30cf6a4 to fa75a8c Compare June 12, 2025 15:06
Comment on lines +429 to +430
emit AllNodeStakesUpdated(operator, newTotalStake);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a revert

@@ -812,8 +803,27 @@ contract AvalancheL1Middleware is IAvalancheL1Middleware, AssetClassRegistry {
if (!balancerValidatorManager.isValidatorPendingWeightUpdate(validationID)) {
revert AvalancheL1Middleware__WeightUpdateNotPending(validationID);
}
// if the completeValidatorWeightUpdate fails, not sure if the previous bool is secure.

// Finish the weight update on the P-Chain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is not super acurate. Could be "Complete weight update upon P-Chain confirmation"

// Check ratio vs. class's min stake, could add an emit here to debug
if (stake / (nodeCount + extraNode) < assetClasses[classId].minValidatorStake) {
if (stake / nodeCount < assetClasses[classId].minValidatorStake) {
emit DebugSecondaryAssetClassCheck(operator, classId, stake, nodeCount, assetClasses[classId].minValidatorStake);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DebugSecondaryAssetClassCheck event should be removed

// Check ratio vs. class's min stake, could add an emit here to debug
if (stake / (nodeCount + extraNode) < assetClasses[classId].minValidatorStake) {
if (stake / nodeCount < assetClasses[classId].minValidatorStake) {
emit DebugSecondaryAssetClassCheck(operator, classId, stake, nodeCount, assetClasses[classId].minValidatorStake);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DebugSecondaryAssetClassCheck event should be removed

Copy link

octane-security-app bot commented Jun 16, 2025

Summary by Octane

New Contracts

  • MockERC20WithDecimals.sol: The contract is an ERC20 token implementation with customizable decimals and an initial supply minted to the deployer.

Updated Contracts

  • DelegatorFactory.sol: Added blacklist check to prevent creation of certain delegator types.
  • VaultFactory.sol: Implemented a blacklist check to prevent upgrades to specific versions.
  • Checkpoints.sol: Replaced self._values[checkpoint._value] with checkpoint._value in return values for efficiency.
  • AvalancheL1Middleware.sol: The smart contract now includes stricter operator validation, improved stake management with epoch delays, and enhanced handling of node updates and removals.
  • MiddlewareVaultManager.sol: Removed address storage in the vaults and adjusted time comparison logic for determining vault activity.
  • Rewards.sol: The contract now supports concurrent fee updates, epoch status tracking, asset class-specific share calculation, prevents reentrancy, and includes epoch funding/distribution constraints.
  • UptimeTracker.sol: The smart contract now fairly distributes leftover uptime seconds across epochs, skipping processed epochs.
  • VaultTokenized.sol: The updated smart contract introduces role consistency checks, deposit limit controls, overflow checks, and redistributes slashed stakes with new events.
  • IDelegatorFactory.sol: Added error for blacklisted versions to enhance validation in the smart contract.
  • IAvalancheL1Middleware.sol: The smart contract was modified to add new error messages, increase validation checks, and introduce a new event for debugging asset class checks.
  • IRewards.sol: Added new error handling for rewards claiming and distribution, including scenarios where epochs are unfunded, already distributed, or exceed share limits, along with a new event for zero rewards claim attempts.
  • IVaultTokenized.sol: The smart contract now includes a slash event with collateral redistribution details.
  • MockAvalancheL1Middleware.sol: The smart contract adds operator management, including enable/disable functions and removal delay, as well as dynamic asset class handling.

🔗 Commit Hash: 6c37d1c

Copy link

octane-security-app bot commented Jun 16, 2025

Overview

Vulnerabilities found: 1                                                                                
Severity breakdown: 1 Medium

Detailed findings

src/contracts/middleware/AvalancheL1Middleware.sol

  • Unrestricted Access to calcAndCacheStakes() Cache Values. See more

🔗 Commit Hash: 6c37d1c
🛡️ Octane Dashboard: All vulnerabilities

@gonzaloetjo gonzaloetjo force-pushed the audit-cyfrin branch 4 times, most recently from 50bce4a to becc395 Compare June 18, 2025 13:31
@gonzaloetjo gonzaloetjo force-pushed the audit-cyfrin branch 2 times, most recently from 4b7b03b to 9891478 Compare June 19, 2025 10:19
@@ -95,6 +96,8 @@ contract Rewards is AccessControlUpgradeable, IRewards {
uptimeTracker = UptimeTracker(uptimeTracker_);
epochDuration = l1Middleware.EPOCH_DURATION();

_checkFees(protocolFee_, operatorFee_, curatorFee_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe redundant with L86-87

@@ -136,10 +139,10 @@ contract Rewards is AccessControlUpgradeable, IRewards {

// Claiming functions
/// @inheritdoc IRewards
function claimRewards(address rewardsToken, address recipient) external {
function claimRewards(address rewardsToken, address recipient) external nonReentrant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did all *claim functions became nonReentrant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment