Unsafe arithmetic in transfer handling

[kevin-valerio]

While performing a transfer, I encountered unsafe arithmetic in ext.rs, which can be triggered using the following PoC

PoC:

#[ink::contract]
mod poc {
    #[ink(storage)]
    pub struct Poc {
        value: bool,
    }

    impl Poc {
        #[ink(constructor)]
        #[ink(payable)]
        pub fn new(init_value: bool) -> Self {
            Self { value: init_value }
        }

        #[ink(message)]
        #[ink(payable)]
        pub fn deposit(&mut self) {
        }

        #[ink(message)]
        pub fn withdraw(&mut self, amount: u128) {
            let transfer = self.env().transfer(self.env().caller(), amount);
            let new_balance = self.env().balance();
        }
    }

    #[cfg(test)]
    mod tests {
        use super::*;
        #[ink::test]
        fn it_works() {
              let mut poc = Poc::new(false);
              ink::env::test::set_value_transferred::<ink::env::DefaultEnvironment>(10);
              poc.deposit(); 
              poc.withdraw(u128::MAX);
        }
    }
}

Output:

thread 'poc::tests::it_works' panicked at .cargo/registry/src/index.crates.io-6f17d22bba15001f/ink_engine-5.1.1/src/ext.rs:118:37:
attempt to subtract with overflow

I haven't been further on the exploitation phase, but I suspect that might be abused by an attacker if the contract is built in release mode, where it will overflow. In debug mode, it just panics like shown above.

Is that known from the team ? Any feedback or help to see if this is actually an issue is appreciated

[kevin-valerio]

Hi @cmichi / @ascjones, excuse me for the ping, but since it might be a security issue, I would be curious to have your take on this. Thank you!

[cmichi]

Hi there @kevin-valerio,

thanks for reporting this!

The issue you encountered is currently only present in master, but not released yet. For reference, here's the culprit.

@davidsemakula Are you interested in looking into removing the annotations?

Are you using ink! master intentionally? It's not yet compatible with any chain besides substrate-contracts-node. We'll do a proper alpha release soon, together with an alpha release for cargo-contract.

[davidsemakula]

Thanks for reporting this @kevin-valerio

The issue you encountered is currently only present in master, but not released yet. For reference, here's the culprit.

@cmichi the lint/diagnostic suppression is only present in master, but the unchecked operations are present in other branches

[davidsemakula]

excuse me for the ping, but since it might be a security issue, I would be curious to have your take on this. Thank you!

@kevin-valerio Michi will confirm but I believe the unchecked operations in question only affect the off-chain testing environment, see here

[kevin-valerio]

excuse me for the ping, but since it might be a security issue, I would be curious to have your take on this. Thank you!

@kevin-valerio Michi will confirm but I believe the unchecked operations in question only affect the off-chain testing environment, see here

Indeed that's also what I think, do you also confirm @cmichi ?
The problem that I see here is that it could create a discrepancy between off-chain and on-chain behaviors, leading to unit tests not properly reflecting the on-chain reality

[cmichi]

Yup, agree with both of you! We certainly have to fix it.