Merkle Trade Move - Taurus Labs

During the creation of a vesting plan, the create function, which can be called by anyone, currently lacks validation for the accuracy of the following arguments: _start_at_sec, _end_at_sec, _initial_amount and _total_amount.

This oversight presents integrity issues, potentially leading to the following scenarios:

• Creation of a vesting plan allowing the user to claim more than the total amount if the provided initial amount exceeds the total amount.

• Creation of a vesting plan with a start time in the past or one that has already expired.

• Creation of a vesting plan with start_at_sec equaling end_at_sec, resulting in a division by zero error when calling get_claimable and related functions (such as cancel and claim).

• Creation of a vesting plan where 'start_at_sec' is greater than end_at_sec, causing an underflow error when calling get_claimable and related functions with the current timestamp exceeding the vesting plan's start time.

Code Location

The create function does not validate the user-provided arguments:

public fun create(
        _user_address: address,
        _start_at_sec: u64,
        _end_at_sec: u64,
        _initial_amount: u64,
        _total_amount: u64,
        _claimable_fa_store_claim_cap: claimable_fa_store::ClaimCapability
    ): (VestingPlan, ClaimCapability, AdminCapability)
    acquires VestingConfig {
        let config = borrow_global_mut<VestingConfig>(@merkle);
        let uid = config.next_uid;
        let vesting_plan = VestingPlan {
            uid,
            user: _user_address,
            start_at_sec: _start_at_sec,
            end_at_sec: _end_at_sec,
            initial_amount: _initial_amount,
            total_amount: _total_amount,
            claimed_amount: 0,
            paused: false,
            claimable_fa_store_claim_cap: _claimable_fa_store_claim_cap
        };
        config.next_uid = config.next_uid + 1;
        (vesting_plan, ClaimCapability { uid }, AdminCapability { uid })
    }

Consider validating user-provided inputs by ensuring the following criteria:

• The initial amount is less than the total amount.

• The current timestamp is before the start time, and the end time is between the start time and a valid upper limit.

Remediation Plan

SOLVED: The Taurus Labs team implemented the recommended checks.

In the esmkl_token module, it was noticed that both E_NOT_AUTHORIZED and E_TOO_SMALL_VEST_AMOUNT errors are assigned the same value, which is 0. This situation can detrimentally impact user experience and complicate the process of identifying the specific cause for a transaction failure.

Code Location

The esmkl_token module exposes two different errors that share the same error code:

/// When signer is not owner of module
const E_NOT_AUTHORIZED: u64 = 0;
/// When vest too small amount
const E_TOO_SMALL_VEST_AMOUNT: u64 = 0;

Consider allocating a distinct error code for each error type to enhance clarity and facilitate precise error identification.

Remediation Plan

SOLVED: The Taurus Labs team solved the issue by assigning the E_TOO_SMALL_VEST_AMOUNT error the value 1.

During the locking process, a validation check determines if a user is eligible for a new vemkl token by assessing the maximum number of tokens they hold. Subsequently, when the mint_vemkl function is called, a token is minted, transferred to the user, and then rendered non-transferrable by invoking object::disable_ungated_transfer with the corresponding transfer reference. This step is intended to "soul bound" the token, as per the accompanying comment. However, since the transfer reference is stored within the VoteEscrowedMKL under the user, If the staking module introduces a new API that provides access to either the VoteEscrowedMKL or the transfer reference contained within it, it creates an avenue for the token owner to call object::enable_ungated_transfer and transfer their tokens using this reference.

The issue arises when vemkl tokens are transferred, as the UserVoteEscrowedMKL lists for both participating users fail to update. Consequently, when the new user attempts to unlock, the operation succeeds as removing a non-existent element from the vector doesn't trigger an error, returning an empty vector instead. However, the vemkl address isn't removed from the initial owner's UserVoteEscrowedMKL list. This discrepancy leads to the following consequences:

• The original vemkl owner accumulates deprecated elements in their UserVoteEscrowedMKL list, potentially rendering them unable to lock and acquire new vemkl tokens.

• The new owner initiating the unlock could surpass the VoteEscrowedMKLConfig.max_num_vemkl limit, as their list doesn't accurately reflect the owned vemkl tokens.

This scenario also exposes the possibility for malicious users to exploit the system by creating multiple accounts, acquiring the maximum allowed vemkl tokens for each account, and then transferring all tokens to their primary accounts. This manipulation results in surpassing the upper bound limit for allowed vemkl tokens, granting a disproportionately strong voting power that could significantly impact the protocol's future.

Code Location

The mint_vemkl function stores the transfer reference for the created vemkl token under the VoteEscrowedMKL resource:

let vemkl = VoteEscrowedMKL {
    lock_time,
    unlock_time: _unlock_time,
    mutator_ref,
    burn_ref,
    transfer_ref,
    mkl_token: mkl_store,
    mkl_delete_ref,
    esmkl_token: esmkl_store, // object address (FungibleStore)
    esmkl_delete_ref,
};

Consider removing the transfer reference from the VoteEscrowedMKL resource to prevent vemkl tokens from being transferred.

Remediation Plan

ACKNOWLEDGED: The Taurus Labs team acknowledged this issue but opted to retain the TransferRef in the user's VoteEscrowedMKL resource for potential future extensibility.

After creating a vesting plan, users have the option to either claim or cancel them. If a user attempts to call cancel before the vesting plan's end date, the current claimable amount will be distributed, and the cancel amount, intended to cover the remaining vesting period, will be returned. This cancel amount can then be utilized, for instance, in esmkl_token::cancel to refund any unused esmkl tokens.

However, it's worth noting that the cancel function can still be invoked even after the vesting plan's conclusion. In such cases, the logic mirrors that of the claim function, albeit with an additional AdminCapability. This introduces an ambiguous behavior where a user can effectively claim their tokens through the cancel function, despite the vesting plan not being canceled in reality.

Code Location

The cancel function enables users to cancel their vesting plans upon conclusion, resulting in a claim instead.

public fun cancel(_vesting_plan: VestingPlan, _claim_cap: ClaimCapability, _admin_cap: AdminCapability): (FungibleAsset, u64) {
    assert!(_vesting_plan.paused == false, E_PAUSED);
    assert!(_vesting_plan.uid == _claim_cap.uid && _vesting_plan.uid == _admin_cap.uid, E_NOT_AUTHORIZED);
    let claimable = get_claimable(&_vesting_plan);
    let claimed_asset = fungible_asset::zero(claimable_fa_store::get_metadata_by_uid(&_vesting_plan.claimable_fa_store_claim_cap));
    if (claimable > 0) {
        fungible_asset::merge(&mut claimed_asset, claim_internal(&mut _vesting_plan, claimable));
    };
    let cancel_amount = _vesting_plan.total_amount - _vesting_plan.claimed_amount;

    (claimed_asset, cancel_amount)
}

Consider implementing a check to ensure that the vesting plan's end date has not been reached before allowing cancellation. This requirement will compel users to claim their plans through the claim function, as intended.

Remediation Plan

SOLVED: The Taurus Labs team revised the logic to ensure that a vesting plan cancellation is permissible only when the cancellation amount exceeds zero.

In the pmkl_token::get_user_season_claimable function, there's a risk that the borrowed RewardInfo resource might not be utilized if the subsequent if clause is triggered. This could lead to unnecessary gas consumption.

Code Location

The get_user_season_claimable function borrows the RewardInfo resource too early, possibly leading to its underutilization.

let reward_info = borrow_global<RewardInfo>(@merkle);
if (!table::contains(&pmkl_info.season_pmkl, _season_number)) {
    return 0
};

Consider borrowing the RewardInfo resource just after the if-clause to ensure its use in the intended context.

if (!table::contains(&pmkl_info.season_pmkl, _season_number)) {
    return 0
};
let reward_info = borrow_global<RewardInfo>(@merkle);

Remediation Plan

SOLVED: The Taurus Labs team implemented the recommended solution.

In the vesting module, both the claim and cancel functions include assertions that compare a boolean expression to false, leading to unnecessary gas consumption.

Code Location

The claim function includes an assertion that compares a boolean expression to false:

public fun claim(_vesting_plan: &mut VestingPlan, _claim_cap: &ClaimCapability): FungibleAsset {
    assert!(_vesting_plan.paused == false, E_PAUSED);

The cancel function includes an assertion that compares a boolean expression to false:

public fun cancel(_vesting_plan: VestingPlan, _claim_cap: ClaimCapability, _admin_cap: AdminCapability): (FungibleAsset, u64) {
    assert!(_vesting_plan.paused == false, E_PAUSED);

Consider implementing the necessary adjustments by adhering to the following principle:

assert!(x == true) -> assert!(x)
assert!(x == false) -> assert!(!x)

Remediation Plan

SOLVED: The Taurus Labs team implemented the recommended adjustments.

Within the modules in scope, several typos were identified.

Code Location

In the staking module, the following typos were identified:

const E_INVALIAD_EPOCH_START_AT: u64 = 5;

string::utf8(b""), // decsription

In the pre_tge_reward module, the following typo was identified:

let seasoun_end_sec = mkl_token::mkl_tge_at() - (DAY_SECONDS * 14 * (tge_season - 3));

Consider implementing the following changes:

E_INVALIAD_EPOCH_START_AT -> E_INVALID_EPOCH_START_AT
decsription -> description
seasoun -> season

Remediation Plan

SOLVED: The Taurus Labs team implemented the recommended changes.

In the get_unlock_amount function within the mkl_token module, there is an unnecessary computation of one month's duration in seconds as DAY_SECONDS * 365 / 12 each time the function is called.

Code Location

The get_unlock_amount function recalculates the duration of one month in seconds every time it is invoked:

let idx = elapsed_time / (DAY_SECONDS * 365 / 12);

Consider adding a new constant named MONTH_SECONDS within the mkl_token module, with a value of 2628000, calculated as DAY_SECONDS * 365 / 12, to avoid redundant computation.

Remediation Plan

ACKNOWLEDGED: The Taurus Labs team acknowledged this issue but decided that using an additional const is an unnecessary use of gas.

Borrowing a resource should ideally be reserved for cases where modification is necessary. However, in the modules in scope, several unnecessary mutable borrows were identified.

Code Location

In the claimable_fa_store module, the following unnecessary mutable borrows were identified:

let claimable_fa_store = borrow_global_mut<ClaimableFaStore>(resource_account);

let claimable_fa_store = borrow_global_mut<ClaimableFaStore>(_claim_cap.resource_account);

let claimable_fa_store = borrow_global_mut<ClaimableFaStore>(_claim_cap.resource_account);

In the mkl_token module, the following unnecessary mutable borrow was identified:

let pool_store = borrow_global_mut<PoolStore<PoolType>>(@merkle);

In the pmkl_token module, the following unnecessary mutable borrow was identified:

let reward_info = borrow_global_mut<RewardInfo>(@merkle);

In the staking module, the following unnecessary mutable borrows were identified:

let config = borrow_global_mut<VoteEscrowedMKLConfig>(@merkle);

let ve_powers = borrow_global_mut<VoteEscrowedPowers>(@merkle);
let user_vemkl = borrow_global_mut<UserVoteEscrowedMKL>(_user_address);

In the protocol_reward module, the following unnecessary mutable borrow was identified:

let user_reward_info = borrow_global_mut<UserRewardInfo<AssetType>>(_user_address);

Consider using borrow_global instead.

Remediation Plan

SOLVED: The Taurus Labs team implemented the recommended changes.

Following AIP-63, the Aptos team is transitioning from the Coin standard to the Fungible Asset (FA) standard. It's advisable to adopt the FA standard from the outset to circumvent potential migration complexities post-contract deployment.

Code Location

The protocol_reward module uses the legacy Coin standard:

use aptos_framework::coin::{Self, Coin};

struct Reward<phantom AssetType> has store {
    registered_at: u64,
    registered_reward_amount: u64,
    coin: Coin<AssetType>
}

let reward = coin::extract(&mut reward.coin, reward_amount);

coin: coin::withdraw<AssetType>(_admin, _amount)

aptos_account::deposit_coins(address_of(_admin), coin::extract_all(&mut reward.coin));
coin::merge(&mut reward.coin, coin::withdraw<AssetType>(_admin, _amount));

Consider using the Fungible Asset (FA) standard instead of the legacy Coin standard.

Remediation Plan

ACKNOWLEDGED: The Taurus Labs team acknowledged this issue but decided against fixing it considering their plan to distribute rewards in zUSDC, which is a USD Coin bridged to Aptos by LayerZero.

In the pmkl_token module, an unused import was identified.

Code Location

The merkle::claimable_fa_store module was imported but never used during the pmkl_token module's unit tests.

#[test_only]
use merkle::claimable_fa_store;

Consider removing the unused import statement.

Remediation Plan

SOLVED: The Taurus Labs team removed the unused import statement.

#[view] decorated functions cannot alter storage; they only read data, providing a safe way to access information without risking state modification. Utilizing the #[view] macro to safeguard read-only functions ensures that any attempt to modify storage inadvertently is prevented, thereby minimizing potential security vulnerabilities. The following read-only functions lack the #[view]decorator:

• pmkl_token::get_current_season_info

• pmkl_token::get_season_user_pmkl

• pmkl_token::get_season_info

• pmkl_token::get_user_season_claimable

• staking::get_epoch_user_vote_power

• mkl_token::get_metadata

• mkl_token::get_unlock_amount

• mkl_token::get_allocation

• esmkl_token::get_metadata

• protocol_reward::user_reward_amount

Consider using the #[view] decorator for the mentioned functions.

Remediation Plan

PENDING: The Taurus Labs team appreciated the recommendation and is open to implementing it in the future.

In the pmkl_token module, the get_season_user_pmkl function unnecessarily contains an assertion that always holds true.

Initially, the function verifies if the provided season number has an entry in the season season_pmkl, returning a default SeasonUserPMKLInfoView resource if not.
After this check, it redundantly verifies again if the season_pmkl table contains an entry for the season number, which is always ensured by the previous return statement.

Code Location

The get_season_user_pmkl function unnecessarily contains an assertion that always holds true.

if (!table::contains(&season_pmkl_info.season_pmkl, _season_number)) {
    return SeasonUserPMKLInfoView {
        season_number: _season_number,
        total_supply: 0,
        user_balance: 0
    }
};
assert!(table::contains(&season_pmkl_info.season_pmkl, _season_number), E_INVALID_SEASON_NUMBER);

Consider removing the redundant check.

Remediation Plan

SOLVED: The Taurus Labs team removed the redundant check.