Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate accounts in cw20 initial balances causes unrecoverable inconsistent state #626

Closed
harryscholes opened this issue Jan 10, 2022 · 3 comments · Fixed by #659
Closed
Assignees
Milestone

Comments

@harryscholes
Copy link
Contributor

If initial_balances contains one or more duplicate accounts, these accounts will have the balance of the final balance saved to state, whilst the total_supply will be the sum over all initial balances provided.

pub fn create_accounts(deps: &mut DepsMut, accounts: &[Cw20Coin]) -> StdResult<Uint128> {
let mut total_supply = Uint128::zero();
for row in accounts {
let address = deps.api.addr_validate(&row.address)?;
BALANCES.save(deps.storage, &address, &row.amount)?;
total_supply += row.amount;
}
Ok(total_supply)
}

Example:

#[test]
fn inconsistent_total_supply_with_duplicate_accounts_in_initial_balances() {
    let mut deps = mock_dependencies(&[]);
    let instantiate_msg = InstantiateMsg {
        name: "Cash Token".to_string(),
        symbol: "CASH".to_string(),
        decimals: 9,
        initial_balances: vec![
            Cw20Coin {
                address: String::from("addr0000"),
                amount: Uint128::from(1u128),
            },
            Cw20Coin {
                address: String::from("addr0000"),
                amount: Uint128::from(1u128),
            },
        ],
        mint: None,
        marketing: None,
    };
    let info = mock_info("creator", &[]);
    let env = mock_env();
    instantiate(deps.as_mut(), env, info, instantiate_msg).unwrap();

    let token_info = query_token_info(deps.as_ref()).unwrap();
    assert_eq!(token_info.total_supply, Uint128::new(2));
    assert_eq!(get_balance(deps.as_ref(), "addr0000"), Uint128::new(1));
}

Should we assert that all accounts in initial_balances are unique? Happy to make a PR for this if there is interest.

NB this was found in an audit of Mars protocol in a contract that was forked from the cw-plus cw20-base contract.

@ethanfrey
Copy link
Member

Thank you for this report. It would be good to assert that all addresses in the initial_balances list only appear once, and fail if there are duplicates

@ethanfrey ethanfrey added this to the v0.11.1 milestone Jan 25, 2022
@maurolacy maurolacy self-assigned this Jan 28, 2022
@maurolacy
Copy link
Contributor

maurolacy commented Jan 28, 2022

@harryscholes do you want to provide a fix for this? I'll assign it to you then.

@maurolacy maurolacy assigned harryscholes and unassigned maurolacy Jan 28, 2022
@harryscholes
Copy link
Contributor Author

harryscholes commented Jan 28, 2022

Thanks @maurolacy. I'll submit a PR in the next couple of weeks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants