Note: I did this contest as a team with Mahdi, we found 15 of total issues but due to some strange judging many valid issues were downgraded. And as a result our 1H, 1M and 1L was confirmed valid in the contest.
The contest validated issues are
- H-02
- M-02
- L-04
- High: 2
- Medium: 3
- Low: 10
In the current implementation, the commission for staking rewards can only be decreased, but this can lead to a loss of commission tokens for the protocol when users claim rewards. Specifically, the protocol does not account for the change in commission rates over time, leading to incorrect commission calculations for periods when the commission rate was higher.
The calculate_rewards function in the pool contract calculates rewards using the current commission rate. Here's a simplified view of how the function works:
fn calculate_rewards(
ref self: ContractState, ref pool_member_info: PoolMemberInfo, updated_index: u64
) -> bool {
let interest: u64 = updated_index - pool_member_info.index;
pool_member_info.index = updated_index;
let rewards_including_commission = compute_rewards_rounded_down(
amount: pool_member_info.amount, interest: interest
);
let commission_amount = compute_commission_amount_rounded_up(
rewards_including_commission: rewards_including_commission, commission: self.commission.read()
);
let rewards = rewards_including_commission - commission_amount;
pool_member_info.unclaimed_rewards += rewards;
true
}This function uses the difference between the updated index and the pool member’s previous index to calculate rewards. It then deducts the commission based on the current commission rate, which could be lower than the rate at the time the rewards were accumulated.
- Initial commission: 2%
- User stakes at index 10.
- After 5 blocks (at index 15), the commission is reduced to 1%.
- The user now wants to claim rewards.
The problem occurs because all rewards will be calculated using the new 1% commission rate, even though rewards between index 10 and 15 should be subject to the 2% commission rate. As a result, the protocol will lose part of its expected commission for that period.
- Alice stakes 100 tokens.
- Total rewards: 10 tokens.
- Commission changes from 2% to 1% halfway through.
Correct Calculation:
- First half: 5 tokens - 2% = 4.9 tokens
- Second half: 5 tokens - 1% = 4.95 tokens
- Total: 4.9 + 4.95 = 9.85 tokens to Alice, 0.15 tokens as commission.
Current Pooling Contract Calculation:
- All rewards: 10 tokens - 1% = 9.9 tokens to Alice, 0.1 tokens as commission.
Result:
- Alice receives 0.05 tokens extra.
- The protocol loses 0.05 tokens in commission.
This discrepancy arises because the contract uses only the current commission rate, ignoring historical changes in the commission rate.
The protocol can lose part of the commission it is entitled to during periods when the commission rate was higher. This could result in a loss of funds for the protocol, especially if commission changes are frequent.
I don't have any solid solution but here is what I think you can do;
Record the commission rate at different indices whenever it changes.
Then when calculating rewards, apply the commission rate that was in effect when the rewards were earned, not just the current commission rate.
A critical issue exists in the exit_delegation_pool_action function, where unclaimed rewards are not reset to zero after they are claimed. This oversight can allow malicious users to repeatedly claim rewards, leading to fund drainage from the pool.
In the exit_delegation_pool_action function, the unclaimed rewards of a pool member are sent to their reward address, but the unclaimed_rewards field is not reset to zero after the rewards are claimed. This allows a user to repeatedly claim their rewards without a proper reset.
The issue arises from this section of the function:
// Claim rewards.
self.send_rewards_to_pool_member(
:pool_member,
reward_address: pool_member_info.reward_address,
amount: pool_member_info.unclaimed_rewards,
:erc20_dispatcher
);After sending rewards, the unclaimed_rewards is not set to zero before the pool member's data is either updated or removed.
This vulnerability allows pool members to repeatedly claim their full unclaimed rewards by calling the exit_delegation_pool_action function multiple times. If the pool member exits with a small amount, the full unclaimed rewards can be claimed repeatedly, draining the pool’s funds.
- A user joins the delegation pool and accumulates rewards.
- The user then triggers the
exit_delegation_pool_actionfunction to claim their rewards, but theunclaimed_rewardsis not reset. - The user can repeatedly call the function with small unstake amounts, claiming the full rewards again and again without resetting the
unclaimed_rewardsvalue.
This repeated claiming can significantly deplete the pool’s rewards and result in substantial financial loss for the system.
To resolve the issue, the contract should reset the unclaimed_rewards to zero immediately after sending the rewards. The following change is recommended:
// Claim rewards.
self.send_rewards_to_pool_member(
:pool_member,
reward_address: pool_member_info.reward_address,
amount: pool_member_info.unclaimed_rewards,
:erc20_dispatcher
);
// Reset unclaimed rewards
pool_member_info.unclaimed_rewards = Zero::zero();
if pool_member_info.amount.is_zero() {
self.remove_pool_member(:pool_member);
} else {
pool_member_info.unpool_time = Option::None;
self.pool_member_info.write(pool_member, Option::Some(pool_member_info));
}This fix ensures that the rewards are properly reset, preventing the exploit from being repeated.
Github
The current implementation of the reward calculation for Starknet staking overestimates daily rewards by not accounting for the extra 0.25 days in a year, resulting in long-term inaccuracies in reward distribution.
-
Overestimation of Daily Rewards:
- The current implementation assumes there are 365 days in a year, whereas a more accurate calculation should consider 365.25 days to account for leap years. This leads to a daily overestimation of rewards by approximately 0.068%.
-
Cumulative Error:
- Over a period of 4 years, this overestimation results in a cumulative error of about 0.274%, which is roughly equivalent to one extra day's worth of rewards being distributed.
-
Long-term Implications:
- This error can lead to the premature depletion of the reward pool and unintended inflation in the token supply, ultimately impacting the tokenomics of the system.
-
Fairness Concerns:
- The reward calculation slightly favors stakers in non-leap years, as they receive more rewards than intended, creating a discrepancy in reward distribution.
- Token Distribution: Slightly faster than intended, which could lead to incorrect projections in token circulation and affect overall tokenomics.
- Reward Pool: Premature depletion of the reward pool due to the over-distribution of rewards.
- Staker Fairness: Unequal distribution of rewards, as stakers in non-leap years receive more than they should.
To correct this issue, it is recommended to use a higher precision constant to represent the length of a year in seconds. The best option is to adjust the SECONDS_IN_YEAR constant to use 365.24 days, which will balance accuracy and simplicity while maintaining integer arithmetic.
Cairo doesn't support floating-point literals like 365.251. However, you can achieve a similar result by using integer arithmetic and adjusting your calculation.
pub const SECONDS_IN_YEAR: u128 = (365 * 24 * 60 * 60) + (6 * 60 * 60); // 365 days + 6 hours
fn calculate_rewards(ref self: ContractState) -> u128 {
let minting_curve_dispatcher = self.minting_curve_dispatcher.read();
let yearly_mint = minting_curve_dispatcher.yearly_mint();
let last_timestamp = self.last_timestamp.read();
let current_time = get_block_timestamp();
self.last_timestamp.write(current_time);
let seconds_diff = current_time - last_timestamp;
yearly_mint * seconds_diff.into() / SECONDS_IN_YEAR
}This adds the extra 6 hours (0.25 days) to account for the leap year every four years 2.
In the current Starknet staking system, there is no mechanism to cancel L1<->L2 messages. If a failure occurs on the L2 side, minted tokens may become stuck in the StarkGate bridge indefinitely, without any ability to recover or cancel the transfer.
When calculating staking rewards on the L2 side, the request_funds_if_needed function checks if additional tokens need to be minted by evaluating whether available funds are less than the unclaimed rewards plus a buffer (if credit < debit + threshold). If more tokens are required, the system requests additional minting by calling send_mint_request_to_l1_staking_minter, which sends a mint request to L1.
On the L1 side, the tick function in the RewardSupplier contract fulfills these L2 requests by minting the required tokens and sending them back to L2 via StarkGate's depositWithMessage function. The msg.value covers gas fees for the L1-to-L2 message processing.
-
Ethereum contracts can send L1-to-L2 messages via the StarkGate bridge, but there is no guarantee the message will be processed by the sequencer on L2. Factors like sudden gas price increases can result in insufficient gas, preventing the message from being processed on L2.
-
While StarkGate requires a minimum of 20k wei for L1 message processing, this only ensures that the message is sent, not that the sequencer will process it. If the L2 side fails to process the message, the minted tokens become stuck in the StarkGate bridge indefinitely.
-
Starknet does not provide a mechanism for cancelling such messages or recovering the tokens once they are stuck. Without the ability to cancel or retry the failed message, the tokens remain in limbo.
If the L2 transaction fails after minting tokens on L1, the tokens remain stuck in the StarkGate bridge with no mechanism to recover them, potentially leading to a permanent loss of tokens.
- Josh stakes tokens on L2, triggering the staking process to request additional tokens from L1 via
calculateStakingRewards. - L1 mints the required tokens and sends them to L2 using the
depositWithMessagefunction. - The
msg.value(at least 20k wei) ensures the L1 transfer succeeds, but the L2 side fails to execute the necessaryon_receiveandl1_handlerfunctions due to sudden spike in the gas price. - The tokens remain stuck in the StarkGate bridge because L2 does not complete the transaction and does not update the internal state (
l1_pending_requested_amountremains unchanged). - Without a cancellation or retry mechanism, the tokens are effectively lost unless manual intervention occurs.
As per the StarkGate documentation, StarkGate provides the depositWithMessageCancelRequest function to recover funds when a message fails to process on the L2 side. It is recommended to implement this function in the Starknet staking protocol to enable token recovery in such cases.
Additionally, ensure that both L1 and L2 cancellation mechanisms are available and robust. Exploring and implementing solutions on both sides will prevent permanent token losses and improve system reliability.
The stake function in the Staking contract checks whether the operational_address has been used, reverting if it has. However, there is a potential DoS issue where a staker can exploit the change_operational_address function to prevent new stakers from using certain operational addresses by sandwiching their transactions. Additionally, there is no check to prevent inactive stakers from using change_operational_address, which could further exacerbate the issue.
The stake function ensures that the operational address provided during staking has not already been assigned to another staker. This check prevents the use of the same operational address for multiple stakers:
fn stake(
ref self: ContractState,
reward_address: ContractAddress,
operational_address: ContractAddress,
amount: u128,
pooling_enabled: bool,
commission: u16,
) -> bool {
self.assert_is_unpaused();
self.roles.only_operator();
self.update_global_index_if_needed();
let staker_address = get_tx_info().account_contract_address;
assert_with_err(self.staker_info.read(staker_address).is_none(), Error::STAKER_EXISTS);
assert_with_err(
self.operational_address_to_staker_address.read(operational_address).is_zero(),
Error::OPERATIONAL_EXISTS
);
// other logic
}If the operational_address is already in use (i.e., not zero in the mapping), the transaction reverts with an error: Error::OPERATIONAL_EXISTS.
The change_operational_address function allows a staker to change their operational address at any time. This function does not check if the staker is in the unstaking process, allowing non-active stakers to change operational addresses as well.
fn change_operational_address(
ref self: ContractState, operational_address: ContractAddress
) -> bool {
self.assert_is_unpaused();
self.roles.only_operator();
self.update_global_index_if_needed();
let staker_address = get_tx_info().account_contract_address;
let mut staker_info = self.get_staker_info(:staker_address);
let old_address = staker_info.operational_address;
staker_info.operational_address = operational_address;
self.staker_info.write(staker_address, Option::Some(staker_info));
self.operational_address_to_staker_address.write(old_address, Zero::zero());
self.operational_address_to_staker_address.write(operational_address, staker_address);
// emit event, etc.
}- A staker could call
change_operational_addressto assign an operational address. - Before another staker stakes with the same operational address, the existing staker could quickly call
change_operational_addressagain, effectively “sandwiching” the new staker’s transaction and causing it to revert. - This prevents new stakers from using valid operational addresses and disrupts the staking process.
Since there is no check on whether the staker is in the unstaking process, even non-active stakers (those who may have withdrawn or are not staking currently) can use this tactic to lock up valid operational addresses.
P.S.: I understand that with the sequencer currently being centralized, the risk is minimal. However, the StarkNet team has repeatedly mentioned that they will be decentralizing the sequencer very soon, which will introduce a public mempool, and this issue will become relevant in that context.
- Denial of Service: A malicious or inactive staker can repeatedly block new stakers from using operational addresses, disrupting the staking process.
- Unfair Advantage: By strategically sandwiching their transactions, a staker can gain control over certain operational addresses and prevent others from staking.
- Exploitation by Non-Active Stakers: Non-active stakers can continue to use
change_operational_address, which should only be available for active stakers, to further perpetuate the DoS attack.
-
Restrict
change_operational_addressto Active Stakers: Implement a check in thechange_operational_addressfunction to ensure that only active stakers (those who are not in the unstaking process) can call this function. -
Introduce a Cooldown or Rate Limiting Mechanism: To prevent rapid switching of operational addresses, introduce a cooldown period or rate-limiting mechanism for the
change_operational_addressfunction to reduce the risk of sandwiching attacks. -
Check for Pending Transactions: Before allowing a change in operational address, verify if any staking or unstaking transactions are pending to prevent front-running or sandwich attacks.
By implementing these recommendations, the protocol can mitigate the risk of DoS and improve the overall fairness and reliability of the staking process.
The unstake_intent function in staking.cairo will revert when contracts are paused, preventing users from initiating the unstaking process during this period. This behavior deviates from standard protocol practices where user withdrawals are typically allowed even when contracts are paused.
In the StarkNet staking protocol, the unstake_intent function includes a check to ensure that the contract is unpaused before allowing users to unstake. This restriction can be seen in the following code snippet:
fn unstake_intent(ref self: ContractState) -> u64 {
self.assert_is_unpaused(); // not allowed to unstake when contracts paused @audit
// extra code
}Typically, protocols allow users to withdraw or unstake their tokens even when contracts are paused to ensure users retain control over their assets. However, in this case, the protocol restricts the ability to unstake during the paused state, potentially locking users' funds.
- Users may be unable to access their staked funds during contract pauses, which could create frustration, especially during emergencies or extended pause periods.
- The inability to unstake during pauses may lead to a loss of user trust in the flexibility of the protocol.
- Code analysis
-
Allow Unstaking During Pauses: Modify the
unstake_intentfunction to allow users to initiate unstaking even when the contracts are paused. This will align with common practice, ensuring that users can always retrieve their assets. -
Pause-Only Critical Functions: Restrict only critical protocol functions during pauses, allowing users to withdraw their funds without affecting the protocol’s stability.
There is a potential issue in the StarkNet staking system related to reward claim front-running due to the 2-3 hours delay in L2->L1 communication for minting new tokens. This delay can lead to unfair reward distribution and exploitation opportunities for faster users who can deplete the available reward pool, leaving others to wait for new tokens.
In the current setup, when a user claims rewards on L2, the rewards are calculated based on the available unclaimed tokens. However, if the available tokens are insufficient, a request is made to mint more tokens from L1. According to starknet this L2->L1 interaction takes around 2-3 hours to finalize, causing a delay in minting new rewards.
- Two users (User A and User B) have earned equal rewards.
- The available unclaimed rewards are insufficient for both users.
- User A claims rewards first, depleting the available pool.
- User B must wait 2-3 hours for the L1 minting process to complete, resulting in a delay in receiving their rewards.
This creates an opportunity for front-running by more active or faster users, giving them an unfair advantage. Additionally, users who experience delays may face the risk of price volatility (e.g., STRK price changes during the waiting period), impacting the value of their rewards.
P.S.: While the sequencer is currently centralized, the StarkNet team has frequently stated that decentralization is on the horizon. Once the sequencer becomes decentralized and a public mempool is introduced, front-running could become a significant concern.
- Delayed Rewards: Slower users face delays in receiving their rewards, leading to frustration and a poor user experience.
- Unfair Advantage: Faster users can front-run reward claims and deplete the available pool, creating an imbalance in reward distribution.
- Price Volatility Risk: Users who claim rewards later may be exposed to STRK price fluctuations, affecting the value of their rewards.
Work on reducing the L2->L1 communication delay to ensure more timely updates to the total supply and reward calculations.
Introduce a fair queuing mechanism for reward claims to prevent faster users from depleting the available reward pool before others can claim their share.
Implement a system where a buffer of unclaimed rewards is maintained on L2, ensuring that rewards can be claimed immediately without waiting for L1 minting.
L-03. Minted tokens will get stuck in starkgate bridge if destination address doesn't fit in felt252
The mintDestination address, which is set by the admin via the setMintDestination function, lacks sanity checks to ensure it is within the size limit of felt252. If this address is set to a value larger than felt252, it can cause a Denial of Service when interacting with StarkNet L2, as certain functions will fail when they attempt to process addresses larger than felt252. It do require(mintDestination_ != 0, "INVALID_MESSAGE_DISPATCHER"); check but the sanity check for address is missing.
The mintDestination address is used when processing L2 requests and minting tokens to send them back to L1 via StarkGate. The issue arises because the setMintDestination function allows the admin to set a mintDestination address of type uint256. However, when interacting with StarkNet L2, addresses must be of type felt252. If the mintDestination is set to a value larger than felt252, the contract will fail to execute certain functions, particularly the #[l1_handler] fn update_total_supply, which will revert, causing the minted tokens to become stuck in the StarkGate bridge.
function setMintDestination(uint256 mintDestination_) internal {
NamedStorage.setUintValueOnce(L2_MINT_DESTINATION_TAG, mintDestination_);
}The mintDestination_ is set as a uint256, which may exceed the maximum size of felt252 used on the StarkNet side.
The following function uses the from_address parameter, which is of type felt252, to verify the L1-to-L2 transaction:
#[l1_handler]
fn update_total_supply(ref self: ContractState, from_address: felt252, total_supply: u128) {
assert_with_err(
from_address == self.l1_staking_minter_address.read(),
Error::UNAUTHORIZED_MESSAGE_SENDER
);
let old_total_supply = self.total_supply.read();
self.total_supply.write(total_supply);
self.emit(Events::TotalSupplyChanged { old_total_supply, new_total_supply: total_supply });
}If the mintDestination set on L1 is larger than the felt252 type, the update_total_supply function will revert, as the address will not fit within the constraints of felt252.
- The admin sets the
mintDestinationto an address larger than the size offelt252. - When a request to mint tokens is processed and sent back to L1, StarkGate sequencer calls the
update_total_supplyfunction. - Since the
mintDestinationis larger thanfelt252, the function will revert, and the tokens will become stuck in the StarkGate bridge, causing a DoS for the transaction.
If the mintDestination is set to an address larger than felt252, the protocol will experience a DoS, as the StarkNet L2 functions that rely on address verification, such as update_total_supply, will revert. This will prevent minted tokens from being transferred back to L1, leaving them stuck in the StarkGate bridge and halting further processing.
PS*: While the likelihood of this issue is low because the mintDestination is set by an admin, it’s important to note that it has not been mentioned in any docs or contest README that the admin is aware of this size limitation. Since a valid ETH address can fit into uint256 but may exceed the felt252 limit, an admin might unintentionally input an invalid address. If the address exceeds the size of felt252, the impact of this issue becomes critical, as it leads to the permanent locking of minted STRK tokens in the StarkGate bridge. Additionally, there is no current mechanism to cancel or revert this message due to the lack of a messaging feature in StarkNet staking.*
Add a sanity check in the setMintDestination function to ensure that the mintDestination address fits within the size limit of felt252. This would prevent any addresses larger than the allowed size from being set, thus avoiding the potential DoS scenario.
The update_global_index_if_needed function in operator.cairo, which is responsible for maintaining accurate reward calculations, can only be called by whitelisted addresses. This restriction poses a risk of delayed reward updates if the whitelisted entities fail to trigger it regularly. This could result in unequal reward distribution and negatively affect the protocol's decentralization goals.
PS: Do note that update_global_index_if_neededfunction is in operator.cairo but it is directly impacting the staking contract
The update_global_index_if_needed function is crucial for ensuring that rewards are calculated correctly and distributed based on up-to-date information.
fn update_global_index_if_needed(ref self: ContractState) -> bool {
self.check_whitelist(get_caller_address()); // ❌ this check is not needed here @audit
self.staking_dispatcher.read().update_global_index_if_needed()
}However, it is currently limited to being callable only by whitelisted addresses. This reliance on a small set of entities creates a potential bottleneck if those whitelisted callers do not update the index frequently enough. Furthermore, this restriction is contrary to decentralization principles, as it prevents regular users or pool members from contributing to the system's maintenance.
- Functionality: Only whitelisted addresses can call
update_global_index_if_needed. - Risk: If whitelisted entities do not call the function frequently, it could lead to larger-than-expected reward accumulations or delays in reward distribution.
- Centralization: Restricting the function to certain addresses increases dependency on specific actors, reducing the decentralization of the protocol.
- Delayed Reward Distribution: If the index isn't updated frequently, users may experience delays in receiving accurate reward calculations, leading to unequal or inconsistent reward payouts.
- Potential Exploits: Larger, unexpected reward claims may occur, creating an imbalance in the reward distribution system.
- Increased Centralization: The restriction to whitelisted addresses creates a centralized point of control for a critical system function, reducing the network's overall decentralization.
Allow any user to call update_global_index_if_needed to ensure the function is triggered regularly, even in low-activity periods.
The reward minting process of the cross-layer communication between ETH (L1) and StarkNet (L2) is broken. The tick function, which is responsible for processing token mint requests from L2, relies on a non-existent l2ToL1MsgHash function in the StarkNet messaging contract. This discrepancy will cause the tick function to revert, effectively breaking the token minting mechanism.
The tick function in the reward supplier contract attempts to use messagingContract().l2ToL1MsgHash to compute and verify the message hash. However, this function does not exist in the StarkNet core messaging contract. The relevant code snippet is as follows:
bytes32 msgHash = messagingContract().l2ToL1MsgHash(
source(),
address(this),
messageReceived
);The StarkNet core messaging contract (StarknetMessaging.sol) does not provide an l2ToL1MsgHash function. Instead, it uses internal hash computation within functions like consumeMessageFromL2.
The impact of this vulnerability is severe:
-
Broken Functionality: The
tickfunction will consistently revert due to the missing function, rendering the token minting process inoperable. -
Stuck Requests: Mint requests originating from L2 will never be processed on L1, leading to a backlog of unfulfilled requests.
-
User Experience: Users expecting to receive minted tokens will face indefinite delays, potentially leading to loss of trust in the system.
-
Economic Implications: The inability to mint tokens as requested could disrupt the token economy and any dependent systems or protocols.
-
System Inconsistency: This issue creates a state inconsistency between L1 and L2, where L2 expects actions that L1 cannot perform.
The root cause appears to be a mismatch between the expected interface of the messaging contract and the actual implementation provided by StarkNet's core messaging contract. This suggests either:
- A misunderstanding of the StarkNet messaging protocol.
- Use of an outdated or incorrect interface for the messaging contract.
- Reliance on a custom function that was not implemented in the final messaging contract.
Utilize the existing functions in the StarkNet core messaging contract to calculate and verify message hashes. For example, adapt the logic used in consumeMessageFromL2 for hash computation.
If a custom l2ToL1MsgHash function is necessary, create a new contract that extends the core StarkNet messaging functionality. This contract should:
- Inherit from or wrap the core StarkNet messaging contract.
- Implement the required
l2ToL1MsgHashfunction. - Be deployed separately and set as the
messagingContractaddress.
The block.timestamp value is widely used in the codebase to validate critical time-dependent operations. However, in Starknet, there are no current restrictions on the return values of the get_block_timestamp() function. This allows the sequencer to submit an arbitrary or incorrect timestamp, which can lead to vulnerabilities in key operations such as staking and unstaking.
In the staking module, the function get_block_timestamp() is used to manage the time-based logic for unstaking, which can create a security risk. When a user calls unstake_intent, it records the intention to unstake and sets the unstake time to 21 days + current time. This means the user cannot unstake until after the 21-day lockup period has passed.
The issue arises when the user calls unstake_action to complete the unstaking process. The unstake_action function checks whether the 21-day period has passed using the following assertion:
assert_with_err(
get_block_timestamp() >= unstake_time,
Error::INTENT_WINDOW_NOT_FINISHED
);Since the sequencer controls the block timestamp and there are no enforced restrictions, it could potentially return an incorrect timestamp, bypassing this check.
-
Bypassing the 21-Day Lockup Period:
A malicious sequencer could return a timestamp that is greater than or equal to theunstake_timeeven if the 21-day period has not passed, allowing the user to unstake prematurely. -
Preventing Unstaking After 21 Days:
Conversely, the sequencer could return a timestamp that is less than the actual time, even after 21 days, preventing the user from unstaking even though the lockup period has elapsed.
The impact of this vulnerability could lead to two potential scenarios:
- Premature Unstaking: A user might be able to unstake before the intended 21-day lockup period, violating the protocol’s intended behavior.
- Delayed Unstaking: A user could be unfairly restricted from unstaking after 21 days due to an incorrect timestamp, leading to user frustration and a loss of trust in the protocol.
Both scenarios can significantly undermine the protocol's integrity and affect user experience.
Minimize reliance on the block.timestamp value for critical, time-sensitive operations. Wherever possible, use alternative methods or implement additional validation checks that do not depend solely on the timestamp.
Keep up to date with the latest Starknet documentation to monitor any changes or improvements to the handling of block.timestamp. As Starknet matures, any improvements to timestamp handling by the sequencer should be integrated into the codebase to ensure security and reliability.
StarkGate enforces a minimum fee for all transactions to cover the costs of L1 → L2 messages. Fees can be estimated using the estimateDepositFeeWei and estimateEnrollmentFeeWei functions. However, when the tick function is called by the user, it does not perform fee estimation, which can lead to transaction failures if the provided fee is less than the required minimum.
In StarkGate, transactions from L1 to L2 require a minimum fee to ensure the message processing cost is covered. This minimum fee can be estimated using the provided estimateDepositFeeWei and estimateEnrollmentFeeWei functions. However, the tick function, which is responsible for minting and sending rewards from L1 to L2, does not call these fee estimation functions to ensure that the correct fee is provided.
As a result, if the user provides a fee that is below the required minimum, the transaction will fail. This issue becomes more significant if there is no clear guidance on the required fee, leading to repeated failures due to insufficient fees.
- Transaction Failure: Every time the
tickfunction is called without proper fee estimation, the transaction will fail if the provided fee is below the required minimum. - Poor User Experience: Users may experience frustration due to repeated transaction failures without understanding the cause, resulting in delays in minting rewards.
- Inefficient Use of Resources: Repeated failed transactions waste resources, including gas fees, without achieving the intended outcome.
-
Implement Fee Estimation in
tick: Update thetickfunction to callestimateDepositFeeWeiorestimateEnrollmentFeeWeibefore proceeding with the transaction. This will ensure that users provide an adequate fee to cover the L1 → L2 message costs. -
Provide Fee Guidance: Include clear documentation or messaging to guide users on the appropriate fee required for calling the
tickfunction, reducing the likelihood of transaction failures.
The MintManager contract relies on a addressToUint256Mapping function in the NamedStorage8 library, which is not implemented. This function is crucial for managing allowances in the contract.
- Critical: The contract will fail to compile due to the missing function.
- If deployed using an alternative compilation method:
- Severe disruption of core contract functionality
- Potential data corruption and inconsistent contract state
- Possible security vulnerabilities due to incorrect storage access
- All allowance-related operations (minting, approving, increasing, decreasing) will malfunction
-
Implement the missing
addressToUint256Mappingfunction in theNamedStorage8library:function addressToUint256Mapping(string memory tag_) internal pure returns (mapping(address => uint256) storage randomVariable) { bytes32 location = keccak256(abi.encodePacked(tag_)); assembly { randomVariable.slot := location } }
-
Alternatively, modify the
allowance()function inMintManagerStorageto use an existing mapping function, such as:function allowance() internal pure returns (mapping(address => uint256) storage) { return NamedStorage.bytes32ToUint256Mapping(ALLOWANCE_TAG); }
This would require converting addresses to bytes32 when accessing the mapping.
-
After implementing the fix, thoroughly test all allowance-related functionalities to ensure correct operation.
There is currently an interdependency issue between the staking contract, the minting_curve contract, and the reward_supplier contract, making it impossible to deploy the staking contract.
The interdependency occurs due to the following:
- Staking Contract: The staking contract requires the
reward_supplieraddress during its construction. - Reward Supplier Contract: The reward_supplier contract, in turn, requires the
stakingandminting_curvecontract addresses for its initialization. - Minting Curve Contract: The minting_curve contract also requires the
stakingcontract address during its construction.
This interdependency leads to a situation where the contracts cannot be deployed in the correct order without causing issues.
Due to the interdependencies, it is impossible to deploy the staking contract and the related contracts, effectively halting the deployment of the entire staking system.
A potential solution is to modify the minting_curve and reward_supplier contracts so that they can set the staking contract address via setters functions after deployment, rather than requiring it in their constructors.
Additionally, the contracts should implement a safeguard mechanism to ensure the staking contract address has been properly set before any functions that rely on it can be used.
This approach breaks the interdependency and ensures that all contracts can be deployed in the correct order, while also maintaining the integrity of the system.
The minting curve in the Starknet staking system adjusts rewards based on the number of stakers, decreasing rewards as more people stake and increasing them when fewer tokens are staked. A staker can potentially exploit this system by front-running large staking transactions, claiming rewards before the reward rate drops due to an influx of new stakers.
This creates an unfair advantage for stakers who can predict incoming large transactions, allowing them to claim higher rewards than they would otherwise receive after the reward adjustment. This could lead to imbalances in the reward distribution and undermine fairness in the staking system.
Introduce a mechanism to prevent or mitigate front-running, such as randomizing reward calculation intervals or setting a lock period for reward claims after significant staking events, ensuring fairer distribution of rewards among all stakers.