Update ics20 contract

[ethanfrey]

Freshen up the contract a bit and add an important improvement to bring it closer to production:

-> Only allowed cw20 tokens can be sent, and they may have a gas limit set to use when transferring them.

As it currently stands, you could send a token from a malicious cw20 contract and when someone sent it back from the destination chain, it would run an infinite loop (or just error). This would kill any relayer trying to send this in a batch with other packets and could (temporarily) make a channel useless until the timeout arrived.

If the cw20 contract is not malicious, the existing code works fine. This allows a gov_contract that can only add new tokens there. This gov_contract cannot disable existing token sends. (Design for minimally intrusive governance that can manage this)

[hashedone]

I don't see two things:

• Setting the gas limit on message execution (it is stored in config, but I don't see where is it applied)
• Test failing if set gas costs affects anything. I think it is doable in MT - create a fake contract which takes very long to exec (some infinite or very long loop of state updates I guess) and then used it with the Transfer. Transfer should fail with with proper error message.

[hashedone]

lgtm

[ueco-jb]

I'd incline to add some (String) with context of operation which failed etc.

I stumbled upon many such errors and each time probably you need to look for answer in specific place in code to understand it

"Only the governance contract can do this: ", {context: contract update}

[hashedone]

Ah, that is why you append a string to a Unauhtorized. I even agree with you, but my request is - do not do it in form: Unauthorized(String). I faced it once or two and I had no idea what is the string - the user name who executed it (may be reasonable on proxying requests), or what? The tuple syntax should be used only when meaning is obvious, otherwise give field a name ;)

[maurolacy]

LGTM.

[maurolacy]

Suggestion: Let's add a gas_limit param to the reply_on fns for ergonomics.

[ethanfrey]

We do, but it takes a non-option (u64) there.
Which makes sense usually when you know you want to set limit to eg. 100.000, but not here

[maurolacy]

Ah, I see there's a with_gas_limit(). Perhaps removing that and taking an Option<u64> gas_limit param in these instead?

[ethanfrey]

It's in cosmwasm_std... 🤷‍♂️

[ethanfrey]

Thanks for the feedback everyone

[giansalex]

@ethanfrey Is possible to change denom from cw20:contract to cw20:contract:denom?
keplr uses this format , i am testing cw20-ics20 in osmosis-frontend and it works perfectly when i use their format .

{
  "denom_traces": [
    {
      "path": "transfer/channel-2",
      "base_denom": "cw20:juno1jue5rlc9dkurt3etr57duutqu7prchqrk2mes2227m52kkrual3qa9lzdd:haz"
    }
  ]
}

• another question, is gas_limit used to prevent if Gov registers a malicious contract?

[ethanfrey]

@ethanfrey Is possible to change denom from cw20:contract to cw20:contract:denom?

Fair suggestion, I can update the text format.
As long as the contract address is in there, it is safe.
BTW, most coins use uppercase denoms this is lower-case. Should I auto-lowercase it?

Update: I looked at the relevant code and realise we only have the address, not more info. We would have to query the contract from that info when doing the conversion (where we have no such access) or earlier and then pass it along. It seems like not as easy as I first thought.

@giansalex can you please open an issue and mention me if you want to follow up on this? I would like to get feedback from the keplr team as well as if this is needed for them or they have another solution

[ethanfrey]

another question, is gas_limit used to prevent if Gov registers a malicious contract?

So gov prevents intentionally malicious contracts that might try bitcoin mining til the tx runs out of gas, thus blocking the relayer from relaying packets.

However, imagine your cw20 token is a complex thing, like cETH from compound, which could call into many more contracts on a transfer. And imagine one of those was doing something bad. Adding a second limit here (beyond blanket whitelist) allows some more control. Like setting to 200.000 to start and be surprised if it ever hits that

[giansalex]

Fair suggestion, I can update the text format. As long as the contract address is in there, it is safe. BTW, most coins use uppercase denoms this is lower-case. Should I auto-lowercase it?

this is the example contract , denom is a param in "RegisterCw20" msg, but can get the original denom.

[ethanfrey]

@giansalex comments on closed PRs are hard to track and I usually just mark all closed PRs as read in my notifications.
Please make an issue to discuss this. And copy over your relevant messages.

It is not a bug in the PR, but a requested feature.