Merge pull request #54 from starknet-id/fix/unchecked_return_erc20

fix: check retrun values of ERC20 transfer & transferFrom functions

src/naming/internal.cairo

@@ -100,8 +100,9 @@ impl InternalImpl of InternalTrait {
         };
 
         // pay the price
-        IERC20CamelDispatcher { contract_address: erc20 }
+        let has_payed = IERC20CamelDispatcher { contract_address: erc20 }
             .transferFrom(get_caller_address(), get_contract_address(), discounted_price);
+        assert(has_payed, 'payment failed');
         // add sponsor commission if eligible
         if sponsor.into() != 0 {
             IReferralDispatcher { contract_address: self._referral_contract.read() }

src/naming/main.cairo

@@ -279,7 +279,7 @@ mod Naming {
             let (hashed_domain, now, expiry) = self.assert_purchase_is_possible(id, domain, days);
             // we need a u256 to be able to perform safe divisions
             let domain_len = self.get_chars_len(domain.into());
-            assert(domain_len != 0, 'domain can\' be empty');
+            assert(domain_len != 0, 'domain can\'t be empty');
             // find domain cost
             let (erc20, price) = IPricingDispatcher {
                 contract_address: self._pricing_contract.read()
@@ -307,7 +307,7 @@ mod Naming {
             let (hashed_domain, now, expiry) = self.assert_purchase_is_possible(id, domain, days);
             // we need a u256 to be able to perform safe divisions
             let domain_len = self.get_chars_len(domain.into());
-            assert(domain_len != 0, 'domain can\' be empty');
+            assert(domain_len != 0, 'domain can\'t be empty');
 
             // check quote timestamp is still valid
             assert(get_block_timestamp() <= max_validity, 'quotation expired');
@@ -729,8 +729,9 @@ mod Naming {
             assert(get_caller_address() == self._admin_address.read(), 'you are not admin');
             let balance = IERC20CamelDispatcher { contract_address: erc20 }
                 .balanceOf(get_contract_address());
-            IERC20CamelDispatcher { contract_address: erc20 }
+            let has_claimed = IERC20CamelDispatcher { contract_address: erc20 }
                 .transfer(get_caller_address(), balance);
+            assert(has_claimed, 'Claim failed');
         }
 
         fn set_discount(ref self: ContractState, discount_id: felt252, discount: Discount) {

src/tests/naming/common.cairo

@@ -87,3 +87,74 @@ fn deploy_stark() -> IERC20CamelDispatcher {
     let stark = utils::deploy(ERC20::TEST_CLASS_HASH, array![]);
     IERC20CamelDispatcher { contract_address: stark }
 }
+
+fn deploy_with_erc20_fail() -> (IERC20CamelDispatcher, IPricingDispatcher, IIdentityDispatcher, INamingDispatcher) {
+    //erc20
+    let eth = utils::deploy(ERC20Fail::TEST_CLASS_HASH, array![]);
+
+    // pricing
+    let pricing = utils::deploy(Pricing::TEST_CLASS_HASH, array![eth.into()]);
+
+    // identity
+    let identity = utils::deploy(Identity::TEST_CLASS_HASH, array![0x123, 0, 0, 0]);
+
+    // naming
+    let admin = 0x123;
+    let address = utils::deploy(
+        Naming::TEST_CLASS_HASH, array![identity.into(), pricing.into(), 0, admin]
+    );
+
+    (
+        IERC20CamelDispatcher { contract_address: eth },
+        IPricingDispatcher { contract_address: pricing },
+        IIdentityDispatcher { contract_address: identity },
+        INamingDispatcher { contract_address: address }
+    )
+}
+
+#[starknet::contract]
+mod ERC20Fail {
+    use starknet::ContractAddress;
+    use openzeppelin::token::erc20::erc20::ERC20Component::InternalTrait as ERC20InternalTrait;
+    use openzeppelin::{
+        token::erc20::{ERC20Component, dual20::DualCaseERC20Impl, ERC20HooksEmptyImpl}
+    };
+
+    component!(path: ERC20Component, storage: erc20, event: ERC20Event);
+
+    #[abi(embed_v0)]
+    impl ERC20Impl = ERC20Component::ERC20Impl<ContractState>;
+    impl ERC20CamelOnlyImpl = ERC20Component::ERC20CamelOnlyImpl<ContractState>;
+    impl ERC20InternalImpl = ERC20Component::InternalImpl<ContractState>;
+
+    #[constructor]
+    fn constructor(ref self: ContractState) {
+        self.erc20.initializer("ether", "ETH");
+        let target = starknet::contract_address_const::<0x123>();
+        self.erc20._mint(target, 0x100000000000000000000000000000000);
+    }
+
+    #[storage]
+    struct Storage {
+        #[substorage(v0)]
+        erc20: ERC20Component::Storage,
+    }
+
+    #[event]
+    #[derive(Drop, starknet::Event)]
+    enum Event {
+        #[flat]
+        ERC20Event: ERC20Component::Event,
+    }
+
+    #[abi(per_item)]
+    #[generate_trait]
+    impl ExternalImpl of ExternalTrait {
+        #[external(v0)]
+        fn transferFrom(
+            ref self: ContractState, sender: ContractAddress, recipient: ContractAddress, amount: u256
+        ) -> bool {
+            false
+        }
+    }
+}

src/tests/naming/test_abuses.cairo

@@ -19,7 +19,7 @@ use naming::interface::naming::{INamingDispatcher, INamingDispatcherTrait};
 use naming::interface::pricing::{IPricingDispatcher, IPricingDispatcherTrait};
 use naming::naming::main::Naming;
 use naming::pricing::Pricing;
-use super::common::deploy;
+use super::common::{deploy, deploy_with_erc20_fail};
 
 #[test]
 #[available_gas(2000000000)]
@@ -300,7 +300,34 @@ fn test_use_reset_subdomains() {
 
 #[test]
 #[available_gas(2000000000)]
-#[should_panic(expected: ('domain can\' be empty', 'ENTRYPOINT_FAILED'))]
+#[should_panic(expected: ('payment failed', 'ENTRYPOINT_FAILED'))]
+fn test_transfer_from_returns_false() {
+    // setup
+    let (eth, pricing, identity, naming) = deploy_with_erc20_fail();
+    let alpha = contract_address_const::<0x123>();
+
+    // we mint the id
+    set_contract_address(alpha);
+    identity.mint(1);
+
+    set_contract_address(alpha);
+    let aller: felt252 = 35683102;
+
+    // we check how much a domain costs
+    let (_, price) = pricing.compute_buy_price(5, 365);
+
+    // we allow the naming to take our money
+    eth.approve(naming.contract_address, price);
+
+    // we buy with no resolver, no sponsor, no discount and empty metadata
+    // in pay_domain transferFrom will return false
+    naming
+        .buy(1, aller, 365, ContractAddressZeroable::zero(), ContractAddressZeroable::zero(), 0, 0);
+}
+
+#[test]
+#[available_gas(2000000000)]
+#[should_panic(expected: ('domain can\'t be empty', 'ENTRYPOINT_FAILED'))]
 fn test_buy_empty_domain() {
     // setup
     let (eth, pricing, identity, naming) = deploy();
@@ -323,4 +350,3 @@ fn test_buy_empty_domain() {
     naming
         .buy(1, empty_domain, 365, ContractAddressZeroable::zero(), ContractAddressZeroable::zero(), 0, 0);
 }
-