abstractsdk / contracts Goto Github PK

Background

This was a finding from the WW security audit.

Canonical address transformations are inefficient

While previously recommended as a best practice, usage of canonical addresses is no longer encouraged. The background is that canonical addresses are no longer stored in a canonical format, so the transformation just adds overhead without much benefit. Additionally, the codebase is more complicated with address transformations.

Recommendation

We recommend removing any transformation from human to canonical addresses and using the new Addr type for validated addresses instead.

Definition of Done

The objective of this task is to move away from the canonical transformations and instead use the Addr type.

The objective of this issue is to implement the MultiSignature cw4 standard in the governance module. The type is already present here.

We will need to update the factory to accept the multis in addition to the monarchy. This issue is dependent on PR#5.

It can make easier development for modules which want their funds to never leave account's balance

Same as title.

They that other contracts are buildt on top of CW721 can be re-used as a base for each kind of different module.

This should replace the current base_dapp implementation and would make testing / making and porting new modules way easier!

When creating a dApp we would like to give the root user the flexibility to set certain parameters yet restrict some others to default values.

This could be done by splitting all dapp InstantiateMsgs to be of the structure:
pub struct GeneralInstantiateMsg { pub base: BaseInstantiateMsg, // Set by manager with default values pub user_input_msg: Binary, // Message provided by root on dApp init }

This way certain variables (like the treasury address and memory address) can't be altered on init.
This would however limit the flexibility as there would be no easy way to add other dapp specific constants to the base init msg.

Another approach could be to implement a factory contract that holds a map of structs with a common trait. The trait would implement a method to fill out a user-input field and keep the rest as default, and create the dapp as such. This would not only be usefull for dApps but for services/contracts we'd like to provide.

Would love some discussion on this!

The dapp_base currently only allows to set one trader. In some cases it could be usefull to have multiple traders allowed.

• Change trader in state.rs to traders: Vec<Addr>
• Add function to add/remove traders

Here we have latest created Response which data will be returned at the end of this action:

Background

This was a finding from the security audit of WW.

The file contracts/treasury/dapps/terraswap/src/dapp_base/common.rs is copied into every dapp contract. This is inefficient, replicates the code and is prone to human error if hard-coded addresses change and have to be modified everywhere.

Recommendation

We recommend putting the file into a package and importing it.

Definition of Done

• dapp_base made into a package
• Imported

While it is advantageous to have upgradable contracts it also adds security risks.

As security is a nr. 1 priority it's important to have a basic guideline around what contract should be upgradable, who should be the person able to upgrade them and how is the upgrade process handled.

In this analysis we can recognize two groups of contracts:

1. Pandora contracts

• The contracts that are direct dependencies for the OS should not be upgradable. Upgrading these contracts should be done by creating a new contract and making it optional to update to the new contract by a config setter.
• All internal contracts should be upgradable through gov process

2. OS contracts

• Almost all contracts should be upgradable with the permissions to change the underlying code_id given to the Manager contract.
• Critical Pandora contract dependencies can be changed by the root user through config setters

We should start using a common Asset type representation.

asset lib

Tracking issue for test-coverage.

In most use cases, one passes only one message to execute on the proxy contract (e.g. here : https://github.com/Kayanski/CavernProtocolModule/blob/main/src/handlers/execute.rs#L37).

However, the executor.execute() function needs a vector as input.

Is it possible to add an AccountAction::execute(app, deps) member directly to facilitate the developers work in case there is only one message to be sent ?

The base_dapp packages/dao_os/src/treasury/dapp_base currently allows the Admin of the contract to change the Treasury and Memory addresses. This should never have to be done so it should be removed.

The relevant tests should also be updated.

A general issue exist when trying to scale on-chain solutions. The problem is the fact that transaction are gas constrained. This means that some atomic transactions, like changing a parameter on a large set of objects, might not be possible in one transaction.

The proposed solution here is to split the operation in different parts. An example of this problem is the asset value calculation inside the Treasury contract. Should the amount of assets and references grow to large, the query becomes impossible to perform.

To mitigate this issue one can implement the following:

1. Lock the set of objects you'd like to perform the action on
2. Perform the action through pagination and keep a record of the last element and an accumulator if needed.
3. When you traversed the whole set you unlock the object.

I've tried to implement a general solution to this as i needed it.

fn page<T,R>(deps: Deps, limit: Option<u32>, f: fn(T, &mut R), mut accumulator: R, start_after: Option<&str>, object: Map<&str,T>) -> (Option<String>, R)
where
T: Serialize + DeserializeOwned,
{
    let limit = limit.unwrap_or(DEFAULT_LIMIT).min(MAX_LIMIT) as usize;
    let start = start_after.map(Bound::exclusive);
    let result: Vec<Vec<u8>> = object
        .range(deps.storage, start, None, Order::Ascending)
        .take(limit)
        .map(|item| { let (key, element) = item.unwrap(); f(element, &mut accumulator); key})
        .collect();

    let last: Option<String> = result.last().clone().map(|key| String::from(std::str::from_utf8(key).unwrap()));
    (last.clone(), accumulator)
}

Limit is the amount of items iterated over in each function call. This should be provided in the call as it depends on how much computation is required for each function call.

The returned key and accumulator can be stored in the contract to be used in the next call.
I think it might be interesting to create a wrapper around MAP something like PagedMap where we implement these functions.

Change it so it takes the module name and an Option as version. If the version is None, it should return the latest version of the package.

Comparing version number can be done using the semver package

We have many ways that Admins can induce changes in our contracts. The objective of this story is to analyze where we can implement time lock functionality. This could both be used for the entirety of Pandora and as a "plugin" that customers could choose to implement in their own contracts.

A description of time lock contracts in general (for use in yield farming at least) can be found here.

In solidity-based contracts, the most widely-used timelock contract was written by Compound and can be found here.

The manager contract should be able to

1. Upgrate existing modules to a new version
2. Update permissions on the currently deployed modules

branch

Account Actions can be created directly from Cosmos Messages using : https://github.com/AbstractSDK/contracts/blob/main/packages/abstract-sdk/src/account_action.rs#L26
However the cosmwasm_std::wasm_execute function that is very nice to use returns a WasmMsg that is not directly convertiable to an AccountAction.

I propose we implement From<WasmMsg> for AccountAction as well to simplify creating proxy messages

The addresses held in the treasury should be stored as a function of the Memory contract storage.

When adding a new vaultasset we should only need to specify the names of the assets . The contract should query the Memory contract for their actual addresses.

This way full control over updates are still handled by the OS admins while instantiation, etc. Remains very easy.

The deposit function should return a CosmosMsg instead of an AccountAction : https://github.com/AbstractSDK/contracts/blob/main/packages/abstract-sdk/src/apis/bank.rs#L128