The Vault data total_shares_minted can be manipulated

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x9e3fb92c28d2f53f2f5379f7cfd275c36695d90da1c00289f181fded901f8c3c
Severity: low

Description:
Description
In src/vault/lib.rs, the Vault's method mint_shares increments the total_shares_minted on the Vault data before actually minting them. The minting process (token.mint) can fail in which case the total_shares_minted will be incorrect. Similarly for the burn_shares method. This exposes future versions of the code to manipulation of the total number of minted shares with a direct impact on the redemption ratio.

Attack Scenario
When Self::env().caller() is different from the token owner, then token.mint would return an error. Alternatively, we could encounter a PSP22Error during the minting process itself. Both cases effectively imply that the tokens won't be minted. However, inside mint_shares the total_shares_minted was already incremented. Hence, the total_shares_minted will be incorrect. This has direct impact on the redemption ration. The same holds for token.burn where early exit with error is also possible, leading to an unsuccessful burn after the total_shares_minted have already been reduced inside burn_shares.

Attachments

1. Proof of Concept (PoC) File

    fn mint_shares(&mut self, amount: Balance, to: AccountId) -> Result<(), VaultError> {
        let mut token: contract_ref!(ShareToken) = self.data.shares_contract.into();
        self.data.total_shares_minted += amount;
        if let Err(e) = token.mint(to, amount) {
            return Err(VaultError::TokenError(e));
        }
        Ok(())
    }
   
    fn burn_shares(&mut self, amount: Balance) -> Result<(), VaultError> {
        let mut token: contract_ref!(PSP22Burnable) = self.data.shares_contract.into();
        self.data.total_shares_minted -= amount;
        if let Err(e) = token.burn(amount) {
            return Err(VaultError::TokenError(e));
        }
        Ok(())
    }
   

2. Revised Code File (Optional)
   Replacing the order in which these two operations take place is sufficient to make these methods more robust.

    fn mint_shares(&mut self, amount: Balance, to: AccountId) -> Result<(), VaultError> {
        let mut token: contract_ref!(ShareToken) = self.data.shares_contract.into();
        if let Err(e) = token.mint(to, amount) {
            return Err(VaultError::TokenError(e));
        }
        self.data.total_shares_minted += amount;
        Ok(())
    }
   
    fn burn_shares(&mut self, amount: Balance) -> Result<(), VaultError> {
        let mut token: contract_ref!(PSP22Burnable) = self.data.shares_contract.into();
        if let Err(e) = token.burn(amount) {
            return Err(VaultError::TokenError(e));
        }
        self.data.total_shares_minted -= amount;
        Ok(())
    }
   

[0xmahdirostami]

Thank you for your submission, but I couldn't figure out the impact.

[veselinas]

In my opinion it is a logical error to update the total_shares_minted before actually successfully minting them. Assuming we have total_shares_minted = N and we initiate the minting of M shares: what the current version of the code is doing is to update total_shares_minted = N+M and then trigger the minting. The minting could fail (which is effectively handled by an early exit). In this case total_shares_minted would be incorrect by as much as M shares. What I propose is to do the minting first and then update the number of total_shares_minted by the amount of successfully minted shares. The total_shares_minted is used in various material calculations and I think we should ensure it is accurate all the time.

More generally, does this mean you are not interested in similar logic-related issues which do not confirm the impact explicitly? (I am new to competitions and still figuring out what are the expectations)

[0xmahdirostami]

In my opinion it is a logical error to update the total_shares_minted before actually successfully minting them. Assuming we have total_shares_minted = N and we initiate the minting of M shares: what the current version of the code is doing is to update total_shares_minted = N+M and then trigger the minting. The minting could fail (which is effectively handled by an early exit). In this case total_shares_minted would be incorrect by as much as M shares. What I propose is to do the minting first and then update the number of total_shares_minted by the amount of successfully minted shares. The total_shares_minted is used in various material calculations and I think we should ensure it is accurate all the time.

More generally, does this mean you are not interested in similar logic-related issues which do not confirm the impact explicitly? (I am new to competitions and still figuring out what are the expectations)

i couldn't completely understand you, could you please provide me with more details and scenarios and a link to code.

[veselinas]

Yes, I will share more details later today when I get access to my PC. I have included the relevant code snippets in my submission to illustrate before (PoC) and after (Revised code). Calling the mint method on the shared token can fail because of authorisation issues (unlikely in the current code version) and a PSP22Error (possible to encounter in the current code version). In the latter case, the minting won't be successful, but total_shares_minted will be updated as if the minting was successful.

[0xmahdirostami]

if a transaction reverts, all state changes will revert to the state before that transaction.

the Vault's method mint_shares increments the total_shares_minted on the Vault data before actually minting them. The minting process (token.mint) can fail in which case the total_shares_minted will be incorrect

if the process of minting reverts, total_shares_minted will revert as well.

the steps in this case aren't important, if any step reverts all steps before that revert as well.

[veselinas]

I see, thanks for clarifying. I was thinking around the line illustrated in the following simplified snippet: if a public method returns an error, it is up to the consumer of this public method to decide what they do with the error. Hence, it all boils down to the lifetime of the Vault/Vault Data objects. In both test functions below we keep the vault/vault data alive after the error is encountered and in test_mint_shares we end up incorrectly counting the unsuccessfully minted shares, whereas in test_mint_shares_revised we only account for the successfully minted shares.

mod vault {

    use std::io::{Error, ErrorKind};

    pub struct Token<'a>{
        owner: &'a String
    }

    impl<'a> Token<'a>{
        pub fn new(owner: &'a String) -> Self {
            Self{ 
                owner: owner
            }
        }

        pub fn mint(&self, to: &String, amount: u32) -> Result<(), std::io::Error > {
            println!("Here we do the minting of {} shares and return an error", amount);
            if self.owner != to {
                return Err(Error::new(ErrorKind::Other, "Error to propagate"));
            }
            Ok(())
        }
    }

    pub struct VaultData<'a>{
        pub admin: &'a String,
        pub shares_contract: Token<'a>,
        pub total_shares_minted: u32,
    }

    pub struct Vault<'a>{
        pub data: VaultData<'a>,
    }

    impl<'a> Vault<'a>{
        fn mint_shares(&mut self, amount: u32, to: &String) -> Result<(), std::io::Error> {
            let token: &Token = &self.data.shares_contract;
            self.data.total_shares_minted += amount;
            if let Err(e) = token.mint(to, amount) {
                return Err(Error::new(ErrorKind::Other, e));
            }
            Ok(())
        }

        fn mint_shares_revised(&mut self, amount: u32, to: &String) -> Result<(), std::io::Error> {
            let token: &Token = &self.data.shares_contract;
            if let Err(e) = token.mint(to, amount) {
                return Err(Error::new(ErrorKind::Other, e));
            }
            self.data.total_shares_minted += amount;
            Ok(())
        }

        pub fn stake(&mut self, amount: u32, to: &String) -> Result<(), std::io::Error> {
            // aim is to call the private method
            self.mint_shares(amount, to)?;
            Ok(())
        }

        pub fn stake_revised(&mut self, amount: u32, to: &String) -> Result<(), std::io::Error> {
            // aim is to call the private method
            self.mint_shares_revised(amount, to)?;
            Ok(())
        }
    }
}
#[cfg(test)]
mod tests {
    use crate::vault::{Token, VaultData, Vault};

    fn setup<'a>(user1: &'a String, user2: &'a String) -> Vault<'a> {
        let data: VaultData = VaultData{
            admin: &user1,
            shares_contract: Token::new( &user2 ),
            total_shares_minted: 0
        };
        Vault{
            data: data
        }
    }

    #[test]
    fn test_mint_shares() {
        let user1: String = String::from("ABC");
        let user2: String = String::from("XYZ");
        let mut vault = setup(&user1, &user2);
        let shares_to_mint_successfully: u32 = 100;
        let shares_to_mint_unsuccessfully: u32 = 200;

        vault.stake( shares_to_mint_successfully, &user2 ).unwrap();
        assert_eq!( vault.data.total_shares_minted, shares_to_mint_successfully );

        let err = vault.stake( shares_to_mint_unsuccessfully, &user1 ).unwrap_err();
        assert_eq!( vault.data.total_shares_minted, shares_to_mint_successfully + shares_to_mint_unsuccessfully);
    }

    fn test_mint_shares_revised() {
        let user1: String = String::from("ABC");
        let user2: String = String::from("XYZ");
        let mut vault = setup(&user1, &user2);
        let shares_to_mint_successfully: u32 = 100;
        let shares_to_mint_unsuccessfully: u32 = 200;

        vault.stake_revised( shares_to_mint_successfully, &user2 ).unwrap();
        assert_eq!( vault.data.total_shares_minted, shares_to_mint_successfully );

        vault.stake_revised( shares_to_mint_unsuccessfully, &user1 ).unwrap_err();
        assert_eq!( vault.data.total_shares_minted, shares_to_mint_successfully);
    }
}

[coreggon11]

@veselinas if a function in ink! panics or returns error the state of the contract is not changed