6.1 The is_in_pool variable should be set to false if all the funds are withdrawn by the delegator
The process_withdrawal(...) function is called to process the withdrawal request after the withdrawal_window_period has passed for a particular delegator. It further calls pool.exit_delegation_pool_action(...) to withdraw the funds from the pool.
fn process_withdrawal(ref self: ContractState) -> u256 {
// ...
let pool = IPoolDispatcher {contract_address: self.pool_contract.read()};
// @audit Withdraw funds from the pool.
let withdrawn_amount: u256 = pool
.exit_delegation_pool_action(get_contract_address())
.into();
// ...
// @audit Send the withdrawn tokens to the user.
strk_token.transfer(self.stake_stark.read(), withdrawn_amount);
// ...
}
The pool.exit_delegation_pool_action(...) function will remove the delegator from the pool if the delegator has withdrawn all the delegated amounts.
fn exit_delegation_pool_action(
ref self: ContractState, pool_member: ContractAddress
) -> Amount {
let mut pool_member_info = self.get_pool_member_info(:pool_member);
// ...
if pool_member_info.amount.is_zero() {
// @audit Delegator will be entirely removed if it no longer
// has any delegated funds.
self.remove_pool_member(:pool_member);
} else {
// @audit Otherwise, update delegator info. The delegator stays
// in the pool. Only this case is considered by process_withdrawal(...).
pool_member_info.unpool_time = Option::None;
self.pool_member_info.write(pool_member, Option::Some(pool_member_info));
}
// ...
}
The problem present in the process_withdrawal(...) function is that it does not consider the case where the delegator is removed entirely from the pool. Because of that, the is_in_pool variable will still be set to true even though the delegator is no longer in the pool.
Later, when the delegate(...) function is called by the StakeStark contract to deposit the funds in the pool, since the delegator is still considered to be in the pool, the add_to_delegation_pool(...) function will be called instead of the enter_delegation_pool(...).
fn delegate(ref self: ContractState, amount: u256) {
// ...
if !self.is_in_pool.read() {
// @audit If the delegator is not in the pool, it must first be registered as a pool member.
let success = pool
.enter_delegation_pool(self.stake_stark.read(), amount_u128);
assert(success, 'Failed to enter delegation pool');
self.is_in_pool.write(true);
} else {
// @audit This function will be called instead since `is_in_pool` is set to true.
// The delegation will fail since the delegator is not a pool member.
let new_total = pool.add_to_delegation_pool(get_contract_address(), amount_u128);
// ...
}
// ...
}
As a result, the add_to_delegation_pool(...) function will revert since the delegator is no longer a pool member, and the delegation process will fail.
During the deposit process, the Stake Stark protocol balances the delegations across all the available delegators. Whenever the batch of deposits is processed, a delegator with the least stake will be chosen to fulfill the delegation request. Since the delegator mentioned in the previous paragraph is still considered available and has a total stake of 0, it will be chosen every time as the best fit to fulfill the delegation.
Since its delegate(...) function will revert due to the abovementioned issue, no funds will be delegated. Thus, even if only one delegator is affected by this problem, no subsequent deposit will be processed and delegated to the delegation pool.
Recommendation
Consider changing the logic to set the is_in_pool variable to false whenever the delegator’s total_stake reaches 0 and it is no longer a pool member.
Resolution
fixed. commit hash: c1b14ab.
6.2 Deposit and Withdrawal actions revert permanently if all delegators are used
The process_batch(...) function executes multiple critical operations for the protocol. The function settles the pending withdrawal and deposit operations, finishes the current un-delegation actions, and accrues rewards.
fn process_batch(ref self: ContractState) {
self.pausable.assert_not_paused();
self.access_control.assert_only_role(OPERATOR_ROLE);
self.reentrancy_guard.start();
self._process_batch();
self._delegator_withdraw();
let total_rewards = self._collect_rewards_from_delegators();
if total_rewards > 0 {
self._distribute_rewards(total_rewards);
}
self.reentrancy_guard.end();
}
The _process_batch(...) function calls _delegate_to_available_delegator(...) or _request_withdrawal_from_available_delegator(...) depending on the pending amount of funds to be withdrawn or deposited. Both of these functions require that at least one delegator is available.
A problem arises because the _delegator_withdraw(...) must be executed in order to make delegators available again. If all the delegators are unavailable at some point, the _delegator_withdraw(...) function must be executed to make delegators available again. However, because there are no available delegators to execute the _delegate_to_available_delegator(...) or _request_withdrawal_from_available_delegator(...), the call to the process_batch(...) function will fail, leaving the protocol in a state where Withdrawals and Deposits are not available.
The protocol can only leave this state if the pending amounts to deposit and withdraw are even.
Recommendation
Consider changing the order of the operations _process_batch(...) and _delegator_withdraw() in the process_batch(...) function. Alternatively, add a function to execute _delegator_withdraw(...) independently.
Resolution
fixed. hash: 1e9cf7e.
6.3 The get_tx_info().account_contract_address can be exploited if user interacts with malicious contract
The deposit(...), _process_withdrawal_request(...) and _process_withdrawals(...) functions use get_tx_info().account_contract_address to get the caller address, which returns the address from which the transaction originated. The deposit(...) function is shown below:
fn deposit(ref self: ContractState, amount: u256, receiver: ContractAddress) -> u256 {
// ...
let caller = get_tx_info().account_contract_address;
// ...
// @audit Transfer the tokens from the account that originated the transaction.
assert(
strk_dispatcher.transfer_from(caller, get_contract_address(), amount),
'Transfer failed'
);
let shares = if receiver.is_zero() {
stSTRK.mint(amount, caller)
} else {
// @audit Mint the shares to the receiver passed in as argument to this function.
stSTRK.mint(amount, receiver)
};
// ...
}
Consider a case where a user has infinite STRK token approval to the stake_stark contract and interacts with any malicious contract. The malicious contract can call the deposit(...) function with receiver as itself. Now, as get_tx_info().account_contract_address will return the user’s address, the malicious contract will receive the shares, which will be minted using the user’s funds.
The request_withdrawal(...) and withdraw(...) functions have similar issues, but for these functions, there isn’t any receiver parameter. Thus, in this case, a malicious contract can withdraw the user’s funds without the user’s consent, and the user will stop getting the rewards. However, funds received from the withdrawal will be sent to the user only, so there will be no loss of funds by withdrawal, unlike deposits.
Recommendation
Consider replacing the instances of get_tx_info().account_contract_address with get_caller_address() to access the user’s address directly. Since the stSTRK contract also calls deposit(...), request_withdrawal(...), and withdraw(...) functions, one of the possible solutions is to add a user’s address as a parameter to already existing functions and only allow for it to be different than the address returned by get_caller_address() if the caller is stSTRK.cairo.
Resolution
fixed. commit hash: 02e022f.
Update from Nethermind Security
The fix for request_withdrawal(...) would break the redeem(...) and withdraw(...) functions from stSTRK. Also, execute_withdrawals still uses tx.origin
Update from the client
fixed. commit hash: 5f6ac8e, 3274ccf.
Update from Nethermind Security
This was fixed by a redesign of the architeture that removes the deposit/withdraw functionalities from the stSTRK token.
6.4 Incorrect best fit algorithm for selecting delegator for withdrawal
The _request_withdrawal_from_available_delegator(...) function is called to request withdrawals from available delegators to fulfill the given amount. It has two loops which go through each of the available delegators and the first one try to select a delegator which is a best fit i.e. a delegator with least delegated stake which can fullfil the withdrawal request. If there isn’t any delegator which can fullfil the given amount then in the second loop, multiple delegators are selected to fullfil the request. But there’s a problem in the implementation of best fit algorithm where if first delegator considered in the loop doesn’t have sufficient delegated stake then it will be considered as best fit and no other delegator will be taken into consideration. Thus, even if a best fit exists, it won’t be selected. The impact of this issue is that multiple delegators would become unavailable which might distrupt withdrawal process i.e. more delay might occur for withdrawal.
fn _request_withdrawal_from_available_delegator(ref self: ContractState, amount: u256) {
let mut remaining_amount = amount;
// First pass: find the best fit delegator
let mut i: u8 = 0;
let mut best_fit_index: u8 = 0;
let mut best_fit_amount: u256 = 0;
while i < self.num_delegators.read() {
if self._is_delegator_available(i) {
let delegator = IDelegatorDispatcher { contract_address: self.delegators.read(i) };
let delegator_stake = delegator.get_total_stake();
// @audit Consider delegators with following stakes: [1,20,40]
// and amount requested for withdrawal be 30
// The best fit in this case would be the first element
// whereas it should be third element.
if delegator_stake > 0 {
if delegator_stake >= remaining_amount
&& (best_fit_amount == 0 || delegator_stake < best_fit_amount) {
best_fit_index = i;
best_fit_amount = delegator_stake;
} else if best_fit_amount == 0 {
best_fit_index = i;
best_fit_amount = delegator_stake;
}
}
}
i += 1;
};
// Second pass: process withdrawal requests
i = 0;
while remaining_amount > 0 && i < self.num_delegators.read() {
// ...
};
assert(remaining_amount == 0, 'Insufficient available funds');
}
Recommendation
Consider revisiting the best fit algorithm to be in accordance with the expected strategy.
Resolution
fixed. hash: b1f15da.
6.5 The STRK token deposits via the stSTRK contract will revert in specific scenarios
Users have two entry points to start participating in the liquid staking process in the Stake Stark protocol. To deposit the STRK tokens, users can call the deposit(...) function directly on either the main StakeStark contract or the stSTRK token contract. Under the hood, the deposit(...) function defined in the liquid staking token contract calls the deposit(...) from the StakeStark contract. This means that it shouldn’t matter which function the user chooses since, eventually, they will follow the same execution path. This, however, is not the case since both functions compute the expected stSTRK token shares differently. The deposit(...) function from the stSTRK contract uses the preview_deposit(...) function, while the deposit(...) from the StakeStark contract uses the preview_mint(...). Both preview functions take the amount of assets as an argument and return the computed shares amount, but they differ in the rounding direction used in the computation process.
The preview_deposit(...) function conforms with the ERC4626 standard and correctly rounds down.
// @audit This function is used in the StakeStark deposit(...)
fn preview_deposit(self: @ContractState, assets: u256) -> u256 {
// @audit This rounds down.
self.convert_to_shares(assets)
}
fn convert_to_shares(self: @ContractState, assets: u256) -> u256 {
if self.total_assets() == 0 {
assets * INITIAL_SHARES_PER_ASSET
} else {
// @audit Rounding down in the preview_deposit(...) happens here.
(assets * self.erc20.total_supply()) / self.total_assets()
}
}
However, the preview_mint(...) function deviates from the ERC4626 standard since, instead of shares, it accepts assets as an input parameter. The function’s logic follows the standard and rounds up, but since the computation is based on assets instead of shares, this leads to an incorrect behavior. In situations when division results in rounding, the preview_mint(...) function will return a higher shares amount than the preview_deposit(...) function for the same amount of assets.
// @audit-issue The preview_mint function in ERC4626 takes
// shares instead of assets. Because of that, the rounding direction
// in the computation below is incorrect. It rounds up instead of down.
fn preview_mint(self: @ContractState, assets: u256) -> u256 {
if self.erc20.total_supply() == 0 {
assets * INITIAL_SHARES_PER_ASSET
} else {
// @audit This rounds up.
(assets * self.erc20.total_supply() + self.total_assets() - 1)
/ self.total_assets()
}
}
Whenever this discrepancy happens, the deposit(...) function from the stSTRK contract will revert.
fn deposit(
ref self: ContractState, assets: u256, receiver: ContractAddress
) -> u256 {
// ...
// @audit shares - rounded down
let shares = self.preview_deposit(assets);
// ...
// @audit shares rounded up
let minted_shares = stake_stark.deposit(assets, receiver);
// @audit-issue Whenever there is a division with a remainder
// and rounding is triggered, this will revert.
assert(minted_shares == shares, 'Shares mismatch');
// ...
}
The impact of this issue is limited to the inconvenience that the users might face in case of unexpected reverts.
Recommendation
Consider revisiting the logic in the preview_mint(...) function so that it follows the ERC4626 standard.
Resolution
fixed. commit hash: df7f3ff
Update from Nethermind Security
The preview_mint(...) function should round-up
Update from the client
fixed. commit hash: ab96ac4
Update from Nethermind Security
This was fixed by a redesign of the architeture that removes the deposit/withdraw functionalities from the stSTRK token.
