Merge pull request #20 from CosmWasm/fix-dos

Fix escrow DoS Attack

contracts/cw20-escrow/schema/details_response.json

@@ -5,6 +5,7 @@
   "required": [
     "arbiter",
     "cw20_balance",
+    "cw20_whitelist",
     "id",
     "native_balance",
     "recipient",
@@ -26,6 +27,13 @@
         "$ref": "#/definitions/Cw20CoinHuman"
       }
     },
+    "cw20_whitelist": {
+      "description": "Whitelisted cw20 tokens",
+      "type": "array",
+      "items": {
+        "$ref": "#/definitions/HumanAddr"
+      }
+    },
     "end_height": {
       "description": "When end height set and block height exceeds this value, the escrow is expired. Once an escrow is expired, it can be returned to the original funder (via \"refund\").",
       "type": [

contracts/cw20-escrow/schema/handle_msg.json

@@ -109,6 +109,16 @@
             }
           ]
         },
+        "cw20_whitelist": {
+          "description": "Besides any possible tokens sent with the CreateMsg, this is a list of all cw20 token addresses that are accepted by the escrow during a top-up. This is required to avoid a DoS attack by topping-up with an invalid cw20 contract. See https://github.com/CosmWasm/cosmwasm-plus/issues/19",
+          "type": [
+            "array",
+            "null"
+          ],
+          "items": {
+            "$ref": "#/definitions/HumanAddr"
+          }
+        },
         "end_height": {
           "description": "When end height set and block height exceeds this value, the escrow is expired. Once an escrow is expired, it can be returned to the original funder (via \"refund\").",
           "type": [

contracts/cw20-escrow/schema/receive_msg.json

@@ -51,6 +51,16 @@
             }
           ]
         },
+        "cw20_whitelist": {
+          "description": "Besides any possible tokens sent with the CreateMsg, this is a list of all cw20 token addresses that are accepted by the escrow during a top-up. This is required to avoid a DoS attack by topping-up with an invalid cw20 contract. See https://github.com/CosmWasm/cosmwasm-plus/issues/19",
+          "type": [
+            "array",
+            "null"
+          ],
+          "items": {
+            "$ref": "#/definitions/HumanAddr"
+          }
+        },
         "end_height": {
           "description": "When end height set and block height exceeds this value, the escrow is expired. Once an escrow is expired, it can be returned to the original funder (via \"refund\").",
           "type": [

contracts/cw20-escrow/src/contract.rs

@@ -74,6 +74,7 @@ pub fn try_create<S: Storage, A: Api, Q: Querier>(
         // there are native coins sent with the message
         native_balance: env.message.sent_funds,
         cw20_balance: vec![],
+        cw20_whitelist: msg.canonical_whitelist(&deps.api)?,
     };
 
     // try to store it, fail if the id was already in use
@@ -93,6 +94,12 @@ pub fn try_cw20_create<S: Storage, A: Api, Q: Querier>(
     token: Cw20Coin,
     msg: CreateMsg,
 ) -> StdResult<HandleResponse> {
+    let mut cw20_whitelist = msg.canonical_whitelist(&deps.api)?;
+    // make sure the token sent is on the whitelist by default
+    if !cw20_whitelist.iter().any(|t| t == &token.address) {
+        cw20_whitelist.push(token.address.clone())
+    }
+
     let escrow = Escrow {
         arbiter: deps.api.canonical_address(&msg.arbiter)?,
         recipient: deps.api.canonical_address(&msg.recipient)?,
@@ -102,6 +109,7 @@ pub fn try_cw20_create<S: Storage, A: Api, Q: Querier>(
         // there are native coins sent with the message
         native_balance: vec![],
         cw20_balance: vec![token],
+        cw20_whitelist,
     };
 
     // try to store it, fail if the id was already in use
@@ -142,6 +150,13 @@ pub fn try_cw20_top_up<S: Storage, A: Api, Q: Querier>(
     // this fails is no escrow there
     let mut escrow = escrows_read(&deps.storage).load(id.as_bytes())?;
 
+    // ensure the token is on the whitelist
+    if !escrow.cw20_whitelist.iter().any(|t| t == &token.address) {
+        return Err(StdError::generic_err(
+            "Only accepts tokens on the cw20_whitelist",
+        ));
+    }
+
     // combine these two
     add_cw20_token(&mut escrow.cw20_balance, token);
     // and save
@@ -298,6 +313,8 @@ fn query_details<S: Storage, A: Api, Q: Querier>(
 ) -> StdResult<DetailsResponse> {
     let escrow = escrows_read(&deps.storage).load(id.as_bytes())?;
 
+    let cw20_whitelist = escrow.human_whitelist(&deps.api)?;
+
     // transform tokens
     let cw20_balance: StdResult<Vec<_>> = escrow
         .cw20_balance
@@ -319,6 +336,7 @@ fn query_details<S: Storage, A: Api, Q: Querier>(
         end_time: escrow.end_time,
         native_balance: escrow.native_balance,
         cw20_balance: cw20_balance?,
+        cw20_whitelist,
     };
     Ok(details)
 }
@@ -354,7 +372,8 @@ mod tests {
             arbiter: HumanAddr::from("arbitrate"),
             recipient: HumanAddr::from("recd"),
             end_time: None,
-            end_height: None,
+            end_height: Some(123456),
+            cw20_whitelist: None,
         };
         let sender = HumanAddr::from("source");
         let balance = coins(100, "tokens");
@@ -363,6 +382,23 @@ mod tests {
         assert_eq!(0, res.messages.len());
         assert_eq!(log("action", "create"), res.log[0]);
 
+        // ensure the details is what we expect
+        let details = query_details(&deps, "foobar".to_string()).unwrap();
+        assert_eq!(
+            details,
+            DetailsResponse {
+                id: "foobar".to_string(),
+                arbiter: HumanAddr::from("arbitrate"),
+                recipient: HumanAddr::from("recd"),
+                source: HumanAddr::from("source"),
+                end_height: Some(123456),
+                end_time: None,
+                native_balance: balance.clone(),
+                cw20_balance: vec![],
+                cw20_whitelist: vec![],
+            }
+        );
+
         // approve it
         let id = create.id.clone();
         let env = mock_env(&deps.api, &create.arbiter, &[]);
@@ -405,6 +441,7 @@ mod tests {
             recipient: HumanAddr::from("recd"),
             end_time: None,
             end_height: None,
+            cw20_whitelist: Some(vec![HumanAddr::from("other-token")]),
         };
         let receive = Cw20ReceiveMsg {
             sender: HumanAddr::from("source"),
@@ -417,6 +454,29 @@ mod tests {
         assert_eq!(0, res.messages.len());
         assert_eq!(log("action", "create"), res.log[0]);
 
+        // ensure the whitelist is what we expect
+        let details = query_details(&deps, "foobar".to_string()).unwrap();
+        assert_eq!(
+            details,
+            DetailsResponse {
+                id: "foobar".to_string(),
+                arbiter: HumanAddr::from("arbitrate"),
+                recipient: HumanAddr::from("recd"),
+                source: HumanAddr::from("source"),
+                end_height: None,
+                end_time: None,
+                native_balance: vec![],
+                cw20_balance: vec![Cw20CoinHuman {
+                    address: HumanAddr::from("my-cw20-token"),
+                    amount: Uint128(100)
+                }],
+                cw20_whitelist: vec![
+                    HumanAddr::from("other-token"),
+                    HumanAddr::from("my-cw20-token")
+                ],
+            }
+        );
+
         // approve it
         let id = create.id.clone();
         let env = mock_env(&deps.api, &create.arbiter, &[]);
@@ -508,13 +568,17 @@ mod tests {
         let res = init(&mut deps, env, init_msg).unwrap();
         assert_eq!(0, res.messages.len());
 
+        // only accept these tokens
+        let whitelist = vec![HumanAddr::from("bar_token"), HumanAddr::from("foo_token")];
+
         // create an escrow with 2 native tokens
         let create = CreateMsg {
             id: "foobar".to_string(),
             arbiter: HumanAddr::from("arbitrate"),
             recipient: HumanAddr::from("recd"),
             end_time: None,
             end_height: None,
+            cw20_whitelist: Some(whitelist),
         };
         let sender = HumanAddr::from("source");
         let balance = vec![coin(100, "fee"), coin(200, "stake")];
@@ -548,6 +612,26 @@ mod tests {
         assert_eq!(0, res.messages.len());
         assert_eq!(log("action", "top_up"), res.log[0]);
 
+        // top with a foreign token not on the whitelist
+        // top up with one foreign token
+        let baz_token = HumanAddr::from("baz_token");
+        let base = TopUp {
+            id: create.id.clone(),
+        };
+        let top_up = HandleMsg::Receive(Cw20ReceiveMsg {
+            sender: HumanAddr::from("random"),
+            amount: Uint128(7890),
+            msg: Some(to_binary(&base).unwrap()),
+        });
+        let env = mock_env(&deps.api, &baz_token, &[]);
+        let res = handle(&mut deps, env, top_up);
+        match res.unwrap_err() {
+            StdError::GenericErr { msg, .. } => {
+                assert_eq!(msg, "Only accepts tokens on the cw20_whitelist")
+            }
+            e => panic!("Unexpected error: {}", e),
+        }
+
         // top up with second foreign token
         let foo_token = HumanAddr::from("foo_token");
         let base = TopUp {

contracts/cw20-escrow/src/msg.rs

@@ -1,4 +1,4 @@
-use cosmwasm_std::{Coin, HumanAddr, Uint128};
+use cosmwasm_std::{Api, CanonicalAddr, Coin, HumanAddr, StdResult, Uint128};
 use cw20::Cw20ReceiveMsg;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
@@ -56,6 +56,19 @@ pub struct CreateMsg {
     /// block time exceeds this value, the escrow is expired.
     /// Once an escrow is expired, it can be returned to the original funder (via "refund").
     pub end_time: Option<u64>,
+    /// Besides any possible tokens sent with the CreateMsg, this is a list of all cw20 token addresses
+    /// that are accepted by the escrow during a top-up. This is required to avoid a DoS attack by topping-up
+    /// with an invalid cw20 contract. See https://github.com/CosmWasm/cosmwasm-plus/issues/19
+    pub cw20_whitelist: Option<Vec<HumanAddr>>,
+}
+
+impl CreateMsg {
+    pub fn canonical_whitelist<A: Api>(&self, api: &A) -> StdResult<Vec<CanonicalAddr>> {
+        match self.cw20_whitelist.as_ref() {
+            Some(v) => v.iter().map(|h| api.canonical_address(h)).collect(),
+            None => Ok(vec![]),
+        }
+    }
 }
 
 pub fn is_valid_name(name: &str) -> bool {
@@ -103,6 +116,8 @@ pub struct DetailsResponse {
     pub native_balance: Vec<Coin>,
     /// Balance in cw20 tokens
     pub cw20_balance: Vec<Cw20CoinHuman>,
+    /// Whitelisted cw20 tokens
+    pub cw20_whitelist: Vec<HumanAddr>,
 }
 
 #[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]

contracts/cw20-escrow/src/state.rs

@@ -2,7 +2,8 @@ use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
 use cosmwasm_std::{
-    CanonicalAddr, Coin, Env, Order, ReadonlyStorage, StdError, StdResult, Storage, Uint128,
+    Api, CanonicalAddr, Coin, Env, HumanAddr, Order, ReadonlyStorage, StdError, StdResult, Storage,
+    Uint128,
 };
 use cosmwasm_storage::{bucket, bucket_read, prefixed_read, Bucket, ReadonlyBucket};
 
@@ -25,6 +26,8 @@ pub struct Escrow {
     pub native_balance: Vec<Coin>,
     /// Balance in cw20 tokens
     pub cw20_balance: Vec<Cw20Coin>,
+    /// All possible contracts that we accept tokens from
+    pub cw20_whitelist: Vec<CanonicalAddr>,
 }
 
 #[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
@@ -49,6 +52,13 @@ impl Escrow {
 
         false
     }
+
+    pub fn human_whitelist<A: Api>(&self, api: &A) -> StdResult<Vec<HumanAddr>> {
+        self.cw20_whitelist
+            .iter()
+            .map(|h| api.human_address(h))
+            .collect()
+    }
 }
 
 pub const PREFIX_ESCROW: &[u8] = b"escrow";