Kakeru Contracts - Kakeru

The mint and burn functions of the ve_ninja token contract have some code that is never executed.

The condition if msg_sender.ne(&fund) is repeated twice in both functions: the first time is an access control that, if the condition is met, the transaction is reverted, so the second condition would never be executed (call to RefreshRewards).

In the case of the mint function, this will bypass the vote_config.max_minted check, being able to mint more tokens than the configured maximum.

It is recommended to modify the condition if it is necessary to execute the call to RefreshReward, although it is not strictly necessary since the update_reward function is called at the beginning of the remaining operations.

However, the vote_config.max_minted check should be taken into account.

Remediation Plan

SOLVED: The code fragments that were not executed have been removed. As a result, the max_minted parameter is no longer meaningful, since there are no more places where it is used.

The unstake function of the fund contract allows to unstake and withdraw previously staked tokens.

However, the call to withdraw which is inside the unstake function does not work the first time it is executed since the variable time2fullredemption used in the function get_claim_able_token is not initialized, having the same value as the variable last_withdraw_time_user.

This means that the behavior of the contract is not the same the first time it is executed as it is the following times.

It is recommended to initialize the variable time2fullredemption before executing the withdraw function to have the same behavior in all executions of the contract. Otherwise, this could have unexpected consequences in the off chain modules that communicate with the contract.

Remediation Plan

NOT APPLICABLE: The Kakeru team have explained that this is the expected behavior:

1. When the user deposits tokens into the fund contract, they receive ve_tokens.

2. To redeem tokens, the user first calls unstake. This destroys their ve_tokens and records the start time for linear release. On the first unstake, the withdraw method only records this start time. On subsequent unstakes, the withdraw method settles any previously unwithdrawn tokens.

The execute_bond function of the basset_inj_hub contract allows delegating to the validators a certain amount of staking, as well as to calculate the amount of binj/stinj tokens to be minted in exchange.

At some point in the function, the binj and stinj exchange rates are updated for future transactions, however, the stinj exchange rate is not updated correctly as the update_stinj_exchange_rate function call is only performed if the bond type is BondType::BondRewards, instead of BondType::StInj.

The affected code snippet:

It is recommended to modify the exchange rate update according to the corresponding bond type.

Remediation Plan

NOT APPLICABLE: The Kakeru team have explained that this is the expected behavior:

When a user bonds new INJ, it does not affect the exchange rate of INJ and stINJ. Only claiming INJ and staking again will affect this exchange rate.

bINJ and stINJ serve different purposes. The target exchange rate of bINJ and INJ is 1:1. If the exchange rate changes due to a slash, a peg fee is needed to restore it to 1:1. The exchange rate is updated in real-time so new users can bond at the previous rate. The goal of stINJ is to increase the exchange rate gradually, allowing users to accumulate staking profits. When a user unbonds stINJ, Check_Slashing is called, and any loss is shared equally among all users.

The execute_burn function from the basset_inj_token_binj contract does not call the CheckSlashing entry point of the basset_inj_hub contract like all other Burn calls, leading to an incorrect State value in the Hub contract.

It is recommended to add the CheckSlashing call to the Hub contract in the execute_burn function to keep the binj exchange rate updated after modifying the total_supply of the token.

Remediation Plan

NOT APPLICABLE: The Kakeru team have explained that this is the expected behavior:

The execute_burn function can only be called by the hub contract and does not require checkSlashing to change the exchange rate, which remains unchanged. The execute_burn_from function will destroy bINJ but does not initiate the unbond operation, so no INJ will be obtained. In this case, checkSlashing will distribute the excess INJ to all bINJ holders.

The withdraw function of the fund contract allows withdrawing previously unstaked tokens.

Since this function is used in both internal and external calls, the user entry is needed to specify the user address that receives the withdrawal.

In the case of an external call using the entry point ExecuteMsg::Withdraw, the user input parameter is not checked against the info.sender parameter. Because of this, withdrawal of unlocked tokens can be performed by any user at any time. This means that the legitimate user could lose the opportunity to re-stake if any other user performs the withdrawal using his address.

It is recommended to add some access control for info.sender at the entry point of ExecuteMsg::Withdraw, before calling the withdraw function.

It is recommended to add some access control for info.sender at the entry point of ExecuteMsg::Withdraw, before calling the withdraw function.

Remediation Plan

SOLVED: The withdraw entry point has been modified so that it only accepts the call from the user info.sender.

The _add_single_user function of the dispatcher contract allows configuring the UserState struct, which stores the information related to a user for claiming future rewards.

In this function, the calculation of user_per_lock_amount parameter is done by an integer division, this means that in the case of a remainder, tokens are not taken into account due to truncation.

The user_claim function does not check in the last period if user_state.claimed_lock_amount is equal to user_state.total_user_lock_amount, so the tokens not considered due to truncation will not be unlocked.

The affected code snippet:

It is important to note that the severity of this problem may vary depending on the number of periods configured in the dispatcher contract and the number of decimal places of the claim_token.

Integration test using the test functions provided in the code but modifying the dispatcher configuration periods from 25 to 23.

The total amount claimed for user Tom is 3999999989 instead of 4000000000, which is the user's locked amount.

fn test_claim_tokens() {
    let block_time = 1688128676u64;
    let creator = Addr::unchecked(CREATOR);
    let tom_address = Addr::unchecked("tom");
    let mut app = mock_app(creator.clone(), vec![], Some(block_time));

    // init cw20 token
    let cw20_contract_id = store_cw20_contract(&mut app);
    let cw20instance_msg: cw20_base::msg::InstantiateMsg = mock_cw20_instantiate_msg();
    let cw20_token = app
        .instantiate_contract(
            cw20_contract_id,
            creator.clone(),
            &cw20instance_msg,
            &[], // no funds
            String::from("cw20_token"),
            None,
        )
        .unwrap();

    // init dispatcher contract
    let dispatcher_contract_id = store_dispatcher_contract(&mut app);
    let dispatcher_instance_msg: crate::msg::InstantiateMsg =
        crate::testing::mock_fn::mock_instantiate_msg(cw20_token.clone());
    let dispatcher_contact = app
        .instantiate_contract(
            dispatcher_contract_id,
            creator.clone(),
            &dispatcher_instance_msg,
            &[], // no funds
            String::from("dispatcher_contact"),
            None,
        )
        .unwrap();

    // transfer cw20 token to dispatcher contract
    let transfer_amount = Uint128::from(10_000_000_000_000u128);
    let res = transfer_token(
        &creator,
        &dispatcher_contact,
        &mut app,
        &cw20_token,
        transfer_amount,
    );
    assert!(res.is_ok());

    // not add user yet , so can not claim
    let add_user_msg = mock_add_users_msg();
    let res = add_users(&creator, &mut app, &add_user_msg, &dispatcher_contact);
    assert!(res.is_ok());

    app.update_block(|block| {
        block.time = Timestamp::from_seconds(1688828677 + 100u64 + 86400 * 30 * 26);
        block.height += 1000000u64;
    });

    // tom claim
    let res = user_claim(&tom_address, &mut app, &dispatcher_contact);
    assert!(res.is_ok());

    // check tom token balance
    let tom_info_after = query_user_info(&mut app, &dispatcher_contact, &tom_address);

    let res = get_token_balance(&mut app, &cw20_token, &tom_address);
    let tom_token_balance = Uint256::from(res.balance);
    assert_eq!(
        tom_info_after.state.claimed_lock_amount,
        Uint256::from(tom_info_after.current_period) * tom_info_after.state.user_per_lock_amount
    );

    assert_eq!(tom_info_after.state.last_claimed_period, 23);
    assert_eq!(tom_token_balance, tom_info_after.state.claimed_lock_amount);
  
    assert_eq!(
        tom_info_after.state.last_claimed_period,
        tom_info_after.current_period
    );

    assert_eq!(
        tom_info_after.state.claimed_lock_amount,
        Uint256::from(4_000_000_000u128)
    );
}

It is recommended to modify the calculation of user_per_lock_amount to take into account the remainder in case of non-exact division. This could be done by changing integers to decimals in the calculations, or by including the remaining amount in the last period.

Remediation Plan

SOLVED: The issue has been solved since the entire dispatcher contract has been removed from the workspace.

The register_contracts function does not have an access control associated with an admin/owner address, instead it relies on the situation where the contracts are not yet configured.

Although the probability of accessing the market contract right after instantiation when the contracts are not configured is low, there is a minimal chance that an attacker could replace the contracts with malicious ones.

The vulnerable code:

It is recommended to include an access control associated with the owner/administrator's address, as this operation cannot be undone.

Remediation Plan

SOLVED: An access control has been added to verify that only config.owner_addr can execute this entry point.

There are some time-related parameters that should be checked if they are not set in the past to avoid erroneous calculations.

This concerns the following parameters:

• msg.start_lock_period_time in the update_config function of the dispatcher contract.

• rule_msg.start_linear_release_time in the add_rule_config function of the distribute contract.

It is recommended to check if the start time parameters are not set in the past before saving them, in order not to distort the calculations.

Remediation Plan

SOLVED: The dispatcher contract has been deleted and the rule_msg.start_linear_release_time from the distribute contract is checked before storing it.

The re_stake functionality of the fund contract is partially suppressed due to the withdrawal included in the unstake function.

Since the tokens are automatically withdrawn in the same transaction as the unstake operation, there is no possibility to execute the re-stake of those tokens because the get_claim_able_token function will return zero amount.

The only amount that could be re-staked is the reserved tokens pending of withdrawal that are calcualted by the get_reserved_token_for_vesting.

It is recommended to separate the withdraw function from the unstake operation in order to give the possibility to the user of re-staking those tokens.

Remediation Plan

NOT APPLICABLE: The Kakeru team have explained that this is the expected behavior:

When an unstake operation occurs, the claimable portion is automatically sent to the user. This is by design.

When re_stake operates, the value returned by get_claim_able_token depends on the time elapsed since the last unstake operation. Since unstake releases tokens linearly, the value of get_claim_able_token increases over time. Thus, the re_stake operation will re-stake both released and yet-to-be-released tokens.

The instantiate and add_rule_config functions of the distribute contract allow creating different rules for distributing Ninja tokens to users.

There are no validations on the three amounts configured in a rule: rule_total_amount, start_release_amount and unlock_linear_release_amount.

It should be checked that rule_total_amount = start_release_amount + unlock_linear_release_amount⁣. Otherwise, if the claim is made after the end time, it will not be possible to claim any token as the claimable amount would be higher than the total and the transaction will be reverted.

It is recommended to check that rule_total_amount = start_release_amount + unlock_linear_release_amount in the affected functions to avoid tokens being permanently locked for the user.

Remediation Plan

SOLVED: The validation rule_total_amount = start_release_amount + unlock_linear_release_amount has been added to the instantiate and add_rule_config functions.

The execute_dispatch_rewards function from basset_inj_rewards_dispatcher contract executes the following call to the UpdateGlobalIndex entry point of the basset_inj_reward contract:

However, the format of the entry point in the basset_inj_reward contract is the following:

The airdrop_hooks parameter included in the first call would cause a failure in case the optional value was other than None, reverting the entire transaction from the UpdateGlobalIndex entry point of the basset_inj_hub contract. This would also cause a failure in the redelegations entry point of the basset_inj_validator_registry contract.

Since at the moment all airdrop_hooks values are hardcoded as None or are in commented code fragments, the criticality has been reduced to Informational.

It is recommended to modify the UpdateGlobalIndex message input in the basset_inj_reward contract if a value of airdrop_hooks other than None is to be used in the future.

Remediation Plan

ACKNOWLEDGED: The Kakeru team has acknowledged this issue.

The implementation of the remove_validator function is almost the same as the redelegations function, but with some incorrectness such as not making the call to UpdateGlobalIndex.

Since the redelegations function is complete and correct, the code of the remove_validator function can be trimmed, leaving only the remove operation in the REGISTRY map.

It is recommended to modify the remove_validator function leaving only the REGISTRY removal operation to save some gas.

Remediation Plan

SOLVED: Finally, the Kakeru team has decided to add the UpdateGlobalIndex call to the remove_validator function, so the redelegations function (Redelegate Proxy entry point) remains as a complementary measure.

As a general good practice, it is always recommended to validate addresses entered in contracts to avoid unexpected errors.

The lack of address validation affects the following parameters:

It is recommended to validate the addresses entered the contracts using the deps.api.addr_validate() function or by converting the address to CanonicalAddr type.

Remediation Plan

SOLVED: The issue has been solved for the following addresses:

The SwapToRewardDenom entry point of the basset_inj_reward (staking) contract can only be called from the basset_inj_rewards_dispatcher contract, but there is no call to this entry point in that contract, so it is not needed.

It is recommended to eliminate the unnecessary entry point.

Remediation Plan

SOLVED: The SwapToRewardDenom entry point has been set to Deprecated, generating an Error in case it is called.

The if condition inside execute_convert_to_basset function from basset_converter contract uses the same condition on both sides of the OR operator.

It is recommended to eliminate the repeated conditions for code hygiene.

Remediation Plan

SOLVED: The repeated condition config.native_denom.is_none() has been replaced by config.basset_token_address.is_none().

There is no validation of amount < balance_of in the withdraw function of the staking contract. In case of insufficient balance the code will panic without any error handling.

It is recommended to check if amount < balance_of and handle the error if this condition is met.

Remediation Plan

SOLVED: The error is handled correctly.

The parameter rule_state.last_claim_linear_release_time used in the claim function is not updated after the claim is performed, but is used in the query_claimable_info function to calculate the claimable amount.

This means that the diff_time will always be calculated from the start_linear_release_time, rather than from the point in time at which the last claim was made. Since the previously claimed amount is subtracted from the claimable one, this does not have a major impact on the calculations.

However, it is recommended to refactor the code and remove the unused parameter rule_state.last_claim_linear_release_time.

It is recommended to refactor the code and remove the unused parameter rule_config_state.last_claim_linear_release_time.

Remediation Plan

SOLVED: The query_claimable_info query has been refactored and the rule_config_state.last_claim_linear_release_release_time parameter has been correctly updated.

The stable_pool contract creates a new stable currency during instantiation, however, the metadata configuration is performed in the separate function set_token_metadata, which has its own entry point, thus completing the creation of the stable currency in two transactions.

It is recommended to call the set_token_metadata function during instantiation to avoid a mint call without configured metadata.

Remediation Plan

ACKNOWLEDGED: The Kakeru team has acknowledged this issue.

The treasure contract performs token locking/unlocking operations, but there is no reward in return, so it does not make sense from a business point of view.

It is recommended to review the purpose of the treasure contract and whether it makes sense from a business perspective for the user.

Remediation Plan

SOLVED: The treasure contract has been deleted.

In the add_validator function of the basset_inj_validator_registry contract, the hub_address parameter is not needed since there is no call to the AddValidator entry point in the basset_inj_hub contract.

It is recommended to eliminate those parameters that are not used for code hygiene.

Remediation Plan

SOLVED: The unused hub_address parameter has been removed from the access control. The variable has been deleted in the commit 8e7f0fce4708f7fbfc27fd6e7609f4d65f6cbe05