when a validator is kicked out of the bonded validator set ,unstake funds will remain in the contract

Summary

when a validator is kicked out of the bonded validator set, auto unbonding of all their delegations This portion of the funds will eventually be transferred to the contract and remain in the contract

Vulnerability Detail

in andromeda-validator-staking We can only get the stake funds back in the following ways

call execute_unstake( 100)
- UNSTAKING_QUEUE.push_back(100)
wait UnbondingTime , x/staking transfer funds to andromeda-validator-staking
call execute_withdraw_fund()
- UNSTAKING_QUEUE.pop_front(100)
- transfer 100 to sender from andromeda-validator-staking

but when a validator is kicked out of the bonded validator set, it will auto unbonding of all their delegations This doesn't go through the above process, it will come directly from x/staking transfer funds to andromeda-validator-staking https://github.com/cosmos/cosmos-sdk/tree/main/x/staking#validator when validator from Bonded -> Unbonding

Validator

..

Unbonding: When a validator leaves the active set, either by choice or due to slashing, jailing or tombstoning, an unbonding of all their delegations begins. All delegations must then wait the UnbondingTime before their tokens are moved to their accounts from the BondedPool.

Impact

when a validator is kicked out of the bonded validator set This portion of the funds will eventually be transferred to the contract and remain in the contract

Code Snippet

https://github.com/sherlock-audit/2024-05-andromeda-ado/blob/main/andromeda-core/contracts/finance/andromeda-validator-staking/src/contract.rs#L34

Tool used

Manual Review

Recommendation

in execute_stake() , call ADOContract::default().add_withdrawable_token()

gjaldon commented 3 months ago

@cvetanovv @MxAxM @WangSecurity sorry to bother, but I just realized that this is Medium Severity because the loss of funds is not permanent. The andromeda-validator-staking contract is upgradable, so the contract can be upgraded so the locked funds can be recovered.

In CosmWasm, a contract is upgradeable as long as an admin is set during instantiation. This admin does not need to be set in CosmWasm contracts' code and can be set when instantiating contracts via the cosmos cli.

From the docs:

However, if the contract is instantiated with an admin address, that account can execute migrations to update the contract's code and state.

There was also an incorrect assumption that a contract needs to define a migrate function in one of the duplicates. Not having a migrate function defined in the andromeda-validator-staking does not mean it is not upgradeable. The admin only needs to be set for the contract and only the new contract for andromeda-validator-staking needs to implement the migrate function.

From CosmWasmMigration docs:

During the migration process, the migrate function defined in the new contract is executed and not the migrate function from the old code and therefore it is necessary for the new contract code to have a migrate function defined and properly exported as an entry_point:

Deploying ADO Contracts

ADO instances are deployed by the user or deployed as a bundle of components via the App ADO. The andromeda-validator-staking contract is an example of an ADO contract.

When deploying via App ADO

During instantiation of an App ADO, app components are added via handle_add_app_component().
handle_add_app_component() generates instantiate messages. The admin for each component is set to the sender. This makes every ADO contract deployed by the App ADO upgradeable.

When user deploys ADO instances directly

Contract owners and all admins are TRUSTED. So when users deploy ADO instances, they are contract owners or admins that are trusted to use values that will not cause any issues. This means they will set admin when deploying ADO contracts to make them upgradeable. Deploying instances is a function that is restricted to the owner/admin of that contract instance.

cvetanovv commented 3 months ago

Judging by this comment here, I'm planning to change the severity of #41 and #30 to Medium because the funds can be rescued.

@gjaldon is right that in CosmWasm, contracts can be set upgradeable by the Admin at the beginning. According to the rules, the Admin is expected to take the correct action. It might also mean setting the contracts upgradeable. This is missing as QA in the Readme, so we'll stick to assuming the Admin will make the right decisions.

cowboy0015 commented 2 months ago

withdraw_fund function is now withdrawing the whole balance instead of manually calculating the rewards to claim. Due to various problems including old version compatibility, unable to set custom withdraw address, etc, there is a possibility where funds can be locked inside the contract. To overcome those unexpected locked funds, withdraw_fund is now working as an emergency as its purpose is to send rewards to the recipient set by the owner.