Staking - Altcoinist

In the StakingVault contract, there's a variable shadowing issue in the _deposit function that leads to incorrect calculation of shares. The daysSinceLastDeposit variable is declared twice, with the inner declaration shadowing the outer one. This results in the outer variable being used for calculations instead of the intended inner variable, potentially leading to incorrect share allocations.

- src/StakingVault.sol

uint256 daysSinceLastDeposit = 0;
if (lastDeposit[receiver].timestamp > 0) {
    uint256 daysSinceLastDeposit = Math.min(1 + (block.timestamp - lastDeposit[receiver].timestamp)/(1 days), 599);
}

// Later in the function
for(; i<daysSinceLastDeposit; i++) {
    r = (r*9965)/10000;
}

In this code, the daysSinceLastDeposit used in the for loop is always 0, regardless of the calculation inside the if statement. This is because the inner declaration creates a new variable that shadows the outer one, and this inner variable is not accessible outside the if block.

The following test case demonstrates the impact of the incorrect share calculation due to the shadowed daysSinceLastDepost variable:

  function test_last_deposit_POC() public {
        console.log("========  Day 1  ======== ");
        console.log("[+] Bob buys 1 month Sub and stakes 10,000 ALTT");
        _subscribe(bob, carol, SubscribeRegistry.packages.MONTHLY, 1, 10000e18, address(0), true);
        console.log("Bob's shares: ", IStakingVault(authorVault).balanceOf(bob));

        skip(250 days);

        console.log("========  Day 251  ======== ");
        console.log("[+] Bob buys 1 month Sub and stakes 10,000 ALTT");
        _subscribe(bob, carol, SubscribeRegistry.packages.MONTHLY, 2, 10000e18, address(0), true);
        console.log("Bob's shares: ", IStakingVault(authorVault).balanceOf(bob));
    }

Notice that the days since last value is 0 instead of 250.

Remove the uint256 type declaration from the inner assignment to fix the shadowing issue:

uint256 daysSinceLastDeposit = 0;
if (lastDeposit[receiver].timestamp > 0) {
    daysSinceLastDeposit = Math.min(1 + (block.timestamp - lastDeposit[receiver].timestamp)/(1 days), 599);
}

This change ensures that the daysSinceLastDeposit variable is correctly updated and used in subsequent calculations.

SOLVED: The suggested mitigation was implemented by the Altcoinist team.

A vulnerability exists in the SubscribeRegistry:subscribe() function, allowing subscribers to effectively pay 32% less than the subscription fees to the author by manipulating the referral address. Specifically, a subscriber can set the referral address (ref) to an address they control, which has subscribed to the author either in the present or past. This enables them to exploit the referral system and gain a significant discount.

In this code, the function checks if the ref address is not address(0), and if so, it verifies that the ref has previously subscribed to the author.

if (ref != address(0)) {
    require(subDir[author][ref] != 0, "RNS");
    toAuthor = (basePrice * 4800) / 10000; // 48%
    toPool = (basePrice * 1200) / 10000; // 12%
    toReferer = (basePrice * 3200) / 10000; // 32%
    toTeam = basePrice - toAuthor - toPool - toReferer; // 8%
    weth.safeTransferFrom(sender, ref, toReferer);
} else {
    toAuthor = (basePrice * 8000) / 10000; // 80%
    toPool = (basePrice * 1200) / 10000; // 12%
    toTeam = basePrice - toAuthor - toPool;
}

The issue lies in this check:

require(subDir[author][ref] != 0, "RNS");

This requirement only ensures that the referral address has subscribed at some point in the past, without verifying if the subscription is currently active.

Consequently, a subscriber can:

1. Subscribe with address1 for a month.

2. After the subscription expires, subscribe again using address2, setting address1 as the referral address.

3. Continue to receive a 32% "cashback" by exploiting the referral system, as the check passes due to the historical subscription of address1.

This manipulation reduces the author's share from 80% to 48%, effectively allowing the attacker to pay only 68% of the base price (100% - 32%).

Add the following test to the provided PoC file

    function test_referrer_discount() public {
        address addr1 = makeAddr("addr1");
        address addr2 = makeAddr("addr2");

        console.log("[+] Alice buys 1 month Sub with addr1");
        _subscribe(addr1, carol, SubscribeRegistry.packages.MONTHLY, 1, 0, address(0), true);

        skip(35 days);

        console.log("Addr1 balance: ", IERC20(WETH).balanceOf(addr1));
        console.log("[+] Alice buys 1 month Sub with addr2 setting referrer as addr1");
        _subscribe(addr2, carol, SubscribeRegistry.packages.MONTHLY, 1, 0, addr1, true);
        console.log("Addr1 balance: ", IERC20(WETH).balanceOf(addr1));
    }

Run forge test --mt "test_referrer_discount" -vvv

To fix this issue, the check should enforce that the referral address has an active subscription, not just a historical one. For example:

require(subDir[author][ref] > block.timestamp, "RNS");

This ensures that the referral address must have a current, valid subscription for the referral discount to apply.

SOLVED: The suggested mitigation was applied by the Altcoinist team.

In SubscribeRegistry, after the TGE, WETH is swapped for ALTT before being sent as a "reward" to the author pool.

Below is the relevant code snippet from SubscribeRegistry::_distribute():

    if (altt.totalSupply() > 0) { // Rewards are only sent after TGE
        if (StakingVault(vault).totalSupply() > 0) {
            uint256 alttReceived = swapWethForAltt(toPool);
            TransferHelper.safeApprove(address(altt), vault, alttReceived);
            TransferHelper.safeTransfer(address(altt), vault, alttReceived);
        }
    } else {
        uint256 sent = StakingVault(vault).depositWeth(vault, toPool);
        require(sent == toPool);
    }

After TGE, the first condition checks if the total supply of ALTT is greater than 0, ensuring rewards are only distributed post-TGE. Then, it verifies that the total supply of shares in the author vault is not zero.

Consequently, the fees from the first subscriber after TGE will not be sent to the pool and will remain as WETH in SubscribeRegistry as no shares have been minted yet.

Add the following test to the provided PoC test file:

    function test_fees_loss_POC() public postTGE {
        console.log("[+] Alice buys 1 month Sub and stakes 10,000 ALTT");
        _subscribe(alice, carol, SubscribeRegistry.packages.MONTHLY, 1, 10000e18, address(0), true);

        //console.log("Pool's ALTT Balance: ", altt.balanceOf(authorVault));
        //MONTHLY FEES = 1e18 WETH (~100e18 ALTT)
        //Pool's portion = 12% of MONTHLY = 12e18
        //Pool balance should be ~10,012 (10,000 stake amount + 12 ALTT as fees)
        assertApproxEqAbs(altt.balanceOf(authorVault), 10000e18 + 12e18, 0.2e18);
    }

Run with forge test --mt "test_fees_loss_POC" -vvv

Remove the following condition in the _distribute() function: if (StakingVault(vault).totalSupply() > 0 )

SOLVED: The suggested mitigation was applied by the Altcoinist team.

To stake, a subscriber must have an active subscription to the author.

However, in the StakingVault::_deposit function, the following checks are used to restrict staking:

require(author != address(0), "UI");
require(authorTokenFactory.balanceOf(receiver, uint256(uint160(author))) > 0, "PD");

First, the function ensures the author's address is non-zero, and then checks if the subscriber holds an ERC1155 token corresponding to the author. This token is minted when a user subscribes to the author for the first time. However, this mechanism does not guarantee that the subscriber currently has an active subscription.

Due to this flaw, a past subscriber can stake in the author's pool and accrue rewards without an active subscription. These rewards cannot be redeemed until the subscriber renews the subscription. Once renewed, the subscriber becomes eligible to claim the rewards earned during the inactive subscription period.

Add the following function to the test PoC file:

function test_stake_noSub() public postTGE {
        console.log("========  Day 1  ======== ");
        console.log("[+] Alice buys 1 month Sub and stakes 10,000 ALTT");
        _subscribe(alice, carol, SubscribeRegistry.packages.MONTHLY, 1, 10000e18, address(0), true);

        skip(30 days);

        console.log("========  Day 31 ======== ");
        deal(address(altt), alice, 500e18);
        console.log("[+] Alice stakes 500 ALTT more even if her subscription is expired");
        vm.startPrank(alice);
        altt.approve(address(authorVault), 500e18);
        IERC4626(authorVault).deposit(500e18, alice);
}

Run forge test --mt "test_stake_noSub" -vvv

Add the following check to StakingVault::_deposit():

require(registry.getSubDetails(author, owner) > block.timestamp);

This ensures that the staker has an active subscription before they stake.

SOLVED: The suggested mitigation was applied by the Altcoinist team.

While the user is not subscribed to an author, they must not be able to earn yield in the staking pool.

This is enforced using a penalty system. The penalty is supposed to slash off the rewards earned during the "not subscribed" period, but instead, it merely delays those rewards.

For example, let's assume there is only one staker:

• Alice subscribes for 1 month and stakes 1000 ALTT.

• After 30 days, the yield generated in the vault from fees is 100 ALTT, which fully belongs to Alice.

Now Alice's subscription has ended.

• After 180 days, the yield generated is 1000 ALTT. Alice should not be able to claim this yield since she does not have an active subscription. The penalty system enforces this restriction.

Alice subscribes for 7 months now.

• For the next 7 months, the yield generated is 1200 ALTT.

• She can now claim the rewards from the 30 days and the 7 months (100 ALTT + 1200 ALTT), whereas she should only be able to claim the rewards for the 30 days and the 7 months (100 ALTT + 1200 ALTT), not the 180 days (1000 ALTT).

Add the following test to the provided PoC file

    function test_yield_generation_POC() public postTGE {
        console.log("========  Day 1  ======== ");
        console.log("[+] Alice buys 1 month Sub and stakes 10,000 ALTT");
        _subscribe(alice, carol, SubscribeRegistry.packages.MONTHLY, 1, 10000e18, address(0), true);

        skip(20 days);
        console.log("========  Day 21  ======== ");
        console.log("[*] Simulating total fees accrual in author pool");
        _addReward(100e18);

        skip(180 days);
        console.log("========  Day 201  ======== ");
        console.log("[*] Simulating total fees accrual in author pool");
        _addReward(1000e18);

        console.log("[+] Alice's Unlocked Rewards:", IStakingVault(authorVault).unlockedRewards(alice));

        //To claim all the rewards, alice now subscribes again for 7 months, offset the penalty
        console.log("[+] Alice Resubscribes for 7 months");
        _subscribe(alice, carol, SubscribeRegistry.packages.MONTHLY, 7, 0, address(0), true);

        skip(190 days);
        console.log("========  Day 391  ======== ");
        console.log("[*] Simulating total fees accrual in author pool");
        _addReward(1000e18);

        //The penalty that alice faces is delay in claiming the rewards INSTEAD of her not recieving those rewards in the first place
        console.log("[+] Alice's Unlocked Rewards:", IStakingVault(authorVault).unlockedRewards(alice));
    }

Run forge test --mt "test_yield_generation_POC" -vvv

Modify the penalty logic to slash off the rewards permanently instead of delaying the rewards.

SOLVED: A new logic was implemented which correctly slashes the user's rewards during their inactive time.

The depositWeth function in the StakingVault contract contains a vulnerability that allows malicious actors to reset the reward accrual timestamp for any user. This can be exploited to prevent legitimate users from accruing their rewards, effectively performing a Denial of Service attack on the reward mechanism.

- src/StakingVault.sol

function depositWeth(address receiver, uint256 amount) public nonReentrant returns (uint256) {
    // ... (other checks)
    else {
            // after TGE+LP
            uint256 amountOut = swapWethForAltt(amount);
            _deposit(self, receiver, amountOut, amountOut);
            notifier.notifyWethDeposit(creator, receiver, amount, amountOut);
            return amountOut;
        }
    // ...
}

The vulnerability stems from the fact that anyone can call depositWeth on behalf of any other user, even with a minimal amount (1 wei). This call updates the lastDepost[owner].timestamp to the current block timestamp, which is used to calculate the unlocked rewards.

An attacker can repeatedly call this function with minimal amounts, constantly resetting the timestamp and preventing the target user from accruing significant rewards. This attack can be sustained until the user's subscription expires, at which point they may have lost all their potential rewards.

The provided test case demonstrates how Bob can reset Alice's rewards to zero by depositing just 1 wei of WETH into Alice's staked amount:

    function test_DoS_Rewards_POC() public {
        deal(WETH, bob, 10e18);
        vm.startPrank(bob);
        IERC20(WETH).approve(address(authorVault), 10e18);
        vm.stopPrank();

        console.log("========  Day 1  ======== ");
        console.log("[+] Alice buys 9 month Sub and stakes 10,000 ALTT");
        _subscribe(alice, carol, SubscribeRegistry.packages.MONTHLY, 9, 10000e18, address(0), true);

        skip(20 days);
        console.log("========  Day 21  ======== ");
        console.log("[+] Total Fees Accrued: 100e18");
        _addReward(100e18);

        console.log("[+] Alice's Unlocked Rewards:", IStakingVault(authorVault).unlockedRewards(alice));

        console.log("[+] Bob deposits 1 wei of WETH (converted to ALTT) into Alice's staked amount");
        vm.prank(bob);
        IStakingVault(authorVault).depositWeth(alice, 1);

        console.log("[+] Alice's Unlocked Rewards:", IStakingVault(authorVault).unlockedRewards(alice));
    }

Output:

To address this vulnerability, consider implementing one or more of the following measures:

• Restrict depositWeth to only allow deposits by the receiver themselves:

  require(msg.sender == receiver, "Only the receiver can deposit");

  
  

• Implement a minimum deposit amount that is significant enough to discourage frequent small deposits:

  
  

  require(amount >= MINIMUM_DEPOSIT, "Deposit amount too low");

  
  

SOLVED: A check was added which prevents a user from depositing for anyone else.

if (receiver != _msgSender() && _msgSender() != address(registry) && receiver != self) {
          revert("cannot deposit for else");
}

The code lines mentioned in the References section indicates that a minimum amount of 0 output tokens from the swap is acceptable, opening up the protocol to a loss of funds via MEV bot sandwich attacks.

Allow the caller to specify a maximum slippage or minimum amount of output tokens to be received from the swap, such that the swap will revert if it wouldn't return the caller-specified minimum amount of output tokens.

Also provide a sensible default if the caller doesn't specify a value, but user-specified slippage parameters must always override defaults.

RISK ACCEPTED: The Altcoinist team accepted the risk of this issue.

The SubscribeRegistry contract contains a vulnerability in its subscribe function The function is designed to allow users to subscribe to a service and stake tokens in a single transaction. However, due to the order of operations, the subscription status is updated after the staking operation, leading to a race condition.

function subscribe(address author, address subber, packages package, uint256 qty, uint256 stakeAmount, address ref) external nonReentrant {
    // ... (earlier parts of the function)

    if (stakeAmount > 0) {
        if(altt.totalSupply() > 0) {
            TransferHelper.safeTransferFrom(address(altt), _msgSender(), self, stakeAmount);
            TransferHelper.safeApprove(address(altt), vault, stakeAmount);
            StakingVault(vault).deposit(stakeAmount, subber);
        } else {
            weth.safeTransferFrom(_msgSender(), self, stakeAmount);
            uint256 sent = StakingVault(vault).depositWeth(subber, stakeAmount);
            require(sent == stakeAmount, "failed to stake all tokens");
        }
    }
    
    subDir[author][subber] = expiry;  // Subscription status updated after staking
    
    // ... (remaining parts of the function)
}

The StakingVault::deposit function, which is called during the staking process, checks for an active subscription. As the subscription status is only updated after this call, it causes the transaction to revert, preventing users from subscribing and staking in one call.

Following is the test function which can be added to provided PoC file:

function test_weth_stake_fail_POC() public {
     console.log("[+] Alice buys 1 month Sub and stakes 10,000 ALTT");

     _subscribe(alice, carol, SubscribeRegistry.packages.MONTHLY, 1, 10000e18, address(0), false);
}

To resolve this issue, the order of operations in the subscribe function should be modified. The subscription status should be updated before the staking operation is performed.

function subscribe(address author, address subber, packages package, uint256 qty, uint256 stakeAmount, address ref) external nonReentrant {
    // ... (earlier parts of the function)

    subDir[author][subber] = expiry;  // Update subscription status before staking

    if (stakeAmount > 0) {
        if(altt.totalSupply() > 0) {
            TransferHelper.safeTransferFrom(address(altt), _msgSender(), self, stakeAmount);
            TransferHelper.safeApprove(address(altt), vault, stakeAmount);
            StakingVault(vault).deposit(stakeAmount, subber);
        } else {
            weth.safeTransferFrom(_msgSender(), self, stakeAmount);
            uint256 sent = StakingVault(vault).depositWeth(subber, stakeAmount);
            require(sent == stakeAmount, "failed to stake all tokens");
        }
    }
    
    // ... (remaining parts of the function)
}

SOLVED: The suggested mitigation was applied by the Altcoinist team.

The start is being set when the StakingVault implementation contract is deployed, as it's being assigned in the constructor.

    constructor(IERC20 _altt, SubscribeRegistry _registry)
        ERC20("", "")
        ERC4626(_altt) // duplicate due to IR compilation
    {
        wethDepositSum = 0;
        conversionDone = false;
        weth = IERC20(0x4200000000000000000000000000000000000006);
        swapRouter = IV3SwapRouter(0x2626664c2603336E57B271c5C0b26F421741e481);
        lockSwap = false;
        registry = _registry;
        start = block.timestamp;
    }

This will lead to all the author pools having the start time as when the staking vault implementation was deployed, instead of setting the start time when the specific author pool was created.

Add the following test to provided PoC file:

    function test_clone_invalidStartDate() public {
        vm.startPrank(address(registry));

        //Deploying Alice's Pool
        address aliceVault = factory.createPool(alice);
        IStakingVault(aliceVault).init(aliceVault, alice, "Test", address(factory), address(tokenFactory));
        uint256 aliceStart = IStakingVault(aliceVault).start();

        vm.warp(block.timestamp + 200 days);

        //Deploying Bob's Pool
        address bobVault = factory.createPool(bob);
        IStakingVault(bobVault).init(aliceVault, alice, "Test", address(factory), address(tokenFactory));
        uint256 bobStart = IStakingVault(bobVault).start();

        //Both start dates are same even if bob's pool is deployed after 200 days
        assertEq(aliceStart, bobStart);
    }

Run forge test --mt "test_clone_invalidStartDate" -vvv

Assign the start value in the init function instead of the constructor.

SOLVED: The suggested mitigation was applied by the Altcoinist team.

In the penalizeUser function of the StakingVault contract, there's a potential mismatch between how the penalty is calculated and how it's applied. The penalty is calculated in terms of assets, but it's burned in terms of shares, which could lead to inconsistencies.

- src/StakingVault.sol

if (deposits[owner] < balanceInAssets) {
    penalty = Math.min(
        balanceInAssets - deposits[owner],
        (inactivityRatio * balanceOf(owner)) / 1e18
    );
    _burn(owner, penalty);
}

The penalty is calculated as the minimum of two values:

1. The difference between the balance in assets and the deposits (in assets)

2. A proportion of the user's balance (in shares)

However, the _burn function is then called with this penalty value, which burns shares, not assets.

This mismatch could lead to incorrect penalty applications:

1. When the share price is greater than 1, users might be penalised more heavily than intended.

2. In extreme cases, the contract might attempt to burn more shares than the user has, potentially causing a revert

This inconsistency could lead to unfair penalties.

To address the issue, the penalty calculation should be consistently done in either assets or shares, and then converted if necessary before burning. Here's a suggested fix:

if (deposits[owner] < balanceInAssets) {
    uint256 penaltyInAssets = Math.min(
        balanceInAssets - deposits[owner],
        (inactivityRatio * balanceInAssets) / 1e18
    );
    uint256 penaltyInShares = _convertToShares(penaltyInAssets, Math.Rounding.Up);
    _burn(owner, penaltyInShares);
}

Additionally, consider adding a check to ensure the calculated penalty in shares doesn't exceed the user's balance:

penaltyInShares = Math.min(penaltyInShares, balanceOf(owner));

SOLVED: The suggested mitigation was implemented by the Altcoinist team.

Anyone can call StakingVault::initWethConversion() after the Token Generation Event (TGE) to convert all accumulated WETH into ALTT by initiating a swap.

The function includes the following check:

require(wethBalance >= wethDepositSum && wethDepositSum > 0);

This ensures two conditions:

1. The actual WETH balance of the staking vault must be greater than or equal to the total WETH staked before the TGE.

2. The total WETH staked must be greater than 0.

However, if all users withdraw their WETH before this function is called, the remaining WETH in the contract will be stuck. This means it cannot be converted into ALTT to be distributed as rewards, effectively locking those funds in the contract.

Add the following test to provided PoC file:

    function test_no_WETH_conversion() public {
        console.log("[+] Alice buys Lifetime Sub and stakes 10,000 WETH before TGE");
        _subscribe(alice, carol, SubscribeRegistry.packages.LIFETIME, 1, 1000e18, address(0), true);

        test_init_TGE();
        console.log("[+] All the staked WETH is withdrawn after TGE");
        vm.prank(alice);
        IStakingVault(authorVault).withdrawWeth(10000e18);

        vm.expectRevert();
        IStakingVault(authorVault).initWethConversion();
    }

Run forge test --mt "test_no_WETH_conversion" -vvvv

Remove the require statement as it is redundant. By removing this check, we ensure that WETH can be converted into ALTT and distributed as rewards even if all users have withdrawn their WETH before this function is called.

SOLVED: The suggested mitigation was applied by the Altcoinist team.

In the penalizeUser function of the StakingVault contract, there's an inconsistency in the event emission logic. The notifyPenalizedBoost event is emitted regardless of whether a penalty is actually applied.

- src/StakingVault.sol

// we only penalize rewards
if (deposits[owner] < balanceInAssets) {
    penalty = Math.min(
        balanceInAssets - deposits[owner],
        (inactivityRatio*balanceOf(owner))/1e18
    );
    _burn(owner, penalty);
}
notifier.notifyPenalizedBoost(creator, owner, value);

The event is emitted outside the if block that determines whether a penalty is applied. This means the event will be emitted even in cases where no penalty is actually enforced (i.e., when deposits[owner] >= balanceInAssets).

Move the event emission inside the if block to ensure it only occurs when a penalty is actually applied:

// we only penalize rewards
if (deposits[owner] < balanceInAssets) {
    penalty = Math.min(
        balanceInAssets - deposits[owner],
        (inactivityRatio*balanceOf(owner))/1e18
    );
    _burn(owner, penalty);
    notifier.notifyPenalizedBoost(creator, owner, value);
}

SOLVED: The suggested mitigation was implemented by the Altcoinist team.

The following contracts implements the Ownable pattern:

• AuthorTokenFactory

• PaymentCollector

• SubscribeRegistry

• ALTT

However, the assessment revealed that the solution does not support the two-step-ownership-transfer pattern. The ownership transfer might be accidentally set to an inactive EOA account. In the case of account hijacking, all functionalities get under permanent control of the attacker.

It is recommended to implement a two-step process (Openzeppelin's Ownable2Step.sol) where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of the ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

SOLVED: The suggested mitigation was applied by the Altcoinist team.

The following function lacks address(0) validation for certain parameters:

• PaymentCollector::constructor - _owner, _altt, _registry

• XPRedeem:constructor - _altt , _signer

• ALTT::constructor - initialOwner

• StakingVault::init - _self

  
  

Every input address should be checked not to be zero, especially the ones that could lead to rendering the contract unusable, lock tokens, etc. This is considered a best practice.

SOLVED: The suggested mitigation was applied by Altcoinist team.

The function AuthorTokenFactory::setMintWhitelist() sets the mapping whitelist instead of mintWhitelist

    function setMintWhitelist(address addr, bool val) public onlyOwner {
        require(addr != address(0) && addr != registry);
        whitelist[addr] = val;
    }

It is recommended to modify the function to set mintWhitelist mapping or change the function name to setWhitelist for clarity.

SOLVED: The function name was changed to setMintWhitelist.

In the VaultNotifier contract, there is a function notifyWethDeposit that declares a parameter wethAmount which is never used within the function body.

function notifyWethDeposit(address vault, address addr, uint256 amount, uint256 wethAmount) public onlyVault {
    emit WETHDeposit(vault, addr, amount);
}

To address this issue, consider one of the following options:

• Remove the unused parameter:

function notifyWethDeposit(address vault, address addr, uint256 amount) public onlyVault {
    emit WETHDeposit(vault, addr, amount);
}

• If the wethAmount is intended to be used but was overlooked, update the function to properly utilize this parameter:

event WETHDeposit(address indexed vault, address indexed addr, uint256 amount, uint256 wethAmount);

function notifyWethDeposit(address vault, address addr, uint256 amount, uint256 wethAmount) public onlyVault {
    emit WETHDeposit(vault, addr, amount, wethAmount);
}

ACKNOWLEDGED : The AltcoinIst team acknowledged the issue.

The following contracts currently imports forge-std/console.sol, which is a debugging tool typically used during development and testing:

• ALTT.sol

• StakingFactory.sol

• StakingVault.sol

• SubscribeRegistry.sol

• XPRedeem.sol

This import is redundant in a production environment.

It is recommended to remove the following import statement from the listed contract:

import "forge-std/console.sol";

ACKNOWLEDGED : The AltcoinIst team acknowledged the issue.

The constructor of the contract lacks zero address checks.

Implement zero address checks for the altt and factory parameters in the constructor.

SOLVED: The issue has been resolved by adding validation on the parameters.

The iterateTWAP() function iterates over an unbounded array shares. This iteration occurs at the end of the function:

for (uint256 i = 0; i < shares.length; i++) {
    uint256 ratio = (shares[i].wethAmount * 1e24) / wethSum;
    uint256 out = Math.min(altt.balanceOf(self), (amountOut * ratio) / 1e24);
    IERC20(altt).safeTransfer(shares[i].pool, out);
}

If the shares array grows too large, the gas required to execute this loop could exceed the block gas limit. This would make the iterateTWAP() function impossible to call, effectively breaking this core functionality of the contract.

- Batch Processing: Instead of processing all shares in one transaction, implement a batching mechanism. This would allow processing a fixed number of shares per transaction, spreading the work across multiple blocks if necessary.

- Pagination: Implement a pagination mechanism that processes a subset of shares in each call. This could be done by adding start and end parameters to the function, allowing external callers to iterate through the shares in manageable chunks.

SOLVED: The issue has been resolved by adding function parameters.

The constructor of the StakingVault contract does not perform null address checks for certain parameters. Specifically, the _altt and _registry addresses are not validated to ensure they are not the zero address (0x0).

- src/StakingVault.sol

constructor(IERC20 _altt, SubscribeRegistry _registry)
    ERC20("", "")
    ERC4626(_altt) // duplicate due to IR compilation
{
    // ... other initializations ...
    registry = _registry;
}

Add address validation checks in the constructor to ensure that neither _altt nor _registry is the zero address:

constructor(IERC20 _altt, SubscribeRegistry _registry)
    ERC20("", "")
    ERC4626(_altt)
{
    require(address(_altt) != address(0), "ALTT address cannot be zero");
    require(address(_registry) != address(0), "Registry address cannot be zero");
    
    // ... other initializations ...
    registry = _registry;
}

SOLVED: The suggested mitigation was implemented by the Altcoinist team.

The topUpWeth function in the StakingVault contract does not include an afterLP check. While this function is restricted to the creator, adding this check would align it with best practices seen in other parts of the contract.

- src/StakingVault.sol

function topUpWeth(uint256 amount) public returns (uint256 amountOut) {
    require(_msgSender() == creator, "PD");
    weth.safeTransferFrom(_msgSender(), self, amount);
    amountOut = swapWethForAltt(amount);
    notifier.notifyTopupWeth(creator, amount);
}

The afterLP check is used elsewhere in the contract to ensure operations only occur after the Liquidity Pool is established. Adding this check to topUpWeth would maintain consistency across the contract's functions.

Consider adding the afterLP modifier to the topUpWeth function for consistency.

SOLVED: The suggested mitigation was implemented by the Altcoinist team.

CreatorTokenFactory.whitelist is never used in CreatorTokenFactory.

TWAP.dailySum is never used in TWAP.

Consider removing unused state variables.

SOLVED: The suggested mitigation was implemented by Altcoinist team.