confio / tfi

Contracts for Regulated DeFi on the Tgrade blockchain
Apache License 2.0
9 stars 0 forks source link

Extend whitelist token with "redeem" function as defined in DSO spec #12

Closed ethanfrey closed 3 years ago

ethanfrey commented 3 years ago

The issue and redeem flow is documented in case 2 in this slab document: https://confio.slab.com/posts/trusted-circles-the-basics-mw00vbcr#case-2-issue-and-redeem-tokens

Key section:

The redemption option is used once the fund’s pre-set life time is over. The asset management fund and the token holders arrange for the return of the tokens off-chain, and using the sending address and memo field token issuer can verify who the redeemer is and initiate a bank transfer to pay the holder in fiat for the tokens. The token can use the redeem function at anytime, but should beforehand and off chain arrange for the token return. Please note, that returning the tokens before the end of the fund’s planned life time will usually involve huge fees. This is where enabling a secondary market via our TokenSwap brings a unique benefit for our blockchain users

For this use-case we don't need to worry about security lifetimes or off-chain negotiations, but this gives some context of why we are doing this.

For me the basic flow is:

Complicated, no? But in the end, all you need to do here is add another ExecuteMsg variant that burns tokens and dispatches a certain event. Oh, and document that exact event format in the README

ks-victor commented 3 years ago

This one looks good to me. I like the flow description.

hashedone commented 3 years ago

I have one problem with this mechanism.

As I understand, everyone holding tokens may execute Reedem, with amount of up to tokens amount he holds. However it should not happen without previous agreement. The problem is what if I reedem my tokens with some random code (or I just misstype code)? I would not get my cash back, as there is no proper agreement, I also lost my tokens, and if bank is nice enough I may ask him for re-minting tokens for me because of some missunderstanding. I can imagine this can be a subject for abuses.

My improvement proposal is:

Add additional message like

{
  "register_reedem": {
    "code": "secret-reedem-code-hash",
    "holder": "account-holding-tokens-to-be-reedemed",
    "limit": 1000 (max. amount of tokens to be reedemed with this code, optional)
  }
}

Bank would execute this message right after agreement is signed, would send this message to the token, and info are stored in state. Then reedem operation is executed by holder - if reedem was not registered before, execution fails.

The problem of this solution is that code is somehow exposed before it is used (so possibly voulnerable for some kind of rainbow table attack), however that is why holder field is added (only holder can reedems money with this code). Obviously codes are meant to be single-time use.

Also additional flaw is, that this solution forces token owner to execute additional message on redeem process, so it has to pay gas, but I guess that would be covered in agreement (bank would include cost in its reedem commission).

Another idea which doesn't involve expose code before reedem execution is to instead of burning tokens, make reedem message store redemtion internally, together with its expiration time. Then add another message accept_reedem with the code (send by bank) which would just remove reedemtion information (as it is accepted) assuming, that bank accepted it and wuold payout. Also there should be symetrical reject_reedem which would also remove reedem info, but automatically re-mint tokens on holder account. The think which would protect from accidentally burning tokens would be third message: rollback_reedem, which can be executed by the holder if and only if reedem expired, and bank still didn't accept it. In such case, holder can call it, so it is equivalent of calling reject_reedem by bank.

I see I am making thinkgs more complicated, and possibly I am totally wrong and this is not a problem, but I am trying to put myself in the bad guy position. I don't need to be whitelisted to reedem, so I have nothing to loose. I can perform invalid reedemtion, go to press that bank cheated on me because he allowed me to burn my tokens, and don't want to refund, and put bank in very strange situation from PR POV. Having in mind that reedem mechanism is by desing avaliable for non-whitelisted actors (which were whitelisted before as they hold tokens - they are actually unthrusted at this point) I want to at least note I see this flaw.

ethanfrey commented 3 years ago

This is a fair concern. The original design only has the on-chain attack vector of mistyping (as the process is pushed off-chain). This one solves that but adds complexity.

Doing a 2 step redeem would make eg. #14 much harder. If I ask the AMM to redeem for me, it is rejected, and the money is returned to the AMM, it must somehow know which account to re-issue the liquidity tokens to. And well, it may only get one of the 2 asset pairs so that is hard to add to the LP without doing a swap, and hmmm.. I'm not on the whitelist.

I think this is a good discussion item that @ks-victor should look into. Especially with UI for redeem. I would figure a MVP implementation that works like Burn{} is simple enough to implement and will let us start testing out off-chain processes, we can iterate.

Kain, can you work with @hashedone to get this into a proper slab / product document? These are super important discussions on the final product level. And let me know if you would prefer the simple approach is "good enough" for now (just burn the tokens and emit some event that the bank is supposed to handle). Or we should do nothing here and spend the time for a more proper/complex design before implementing Redeem at all?

I have no idea your product timeframe or design for integrating redeem, so I cannot make that call. But these thoughts definitely should be captured.

hashedone commented 3 years ago

I actually have an idea how to solve #14 with double-step rejection (also commented there briefly), but I didn't went deeply there as I wasn't sure if my concerns are even reasonable and which way would we go with that.

Basically MVP implementation "just burn" makes no harm - later if we add some additional validation (two-step or any other way), then executions using those mechanisms would be protected, and those which was executed prior to it are vulnerable, but it is ok as long if token provider is aware of this. Also it is possible that token provider decide, that handling potential risk here is cheaper that confirming all reedems, so allow such a way is not bad thing.

ks-victor commented 3 years ago

@hashedone very interesting read. thanks.

My take on the problem, I have been thrown out of the trusted circle, so there is probably a problem with me. In this scenario, I typed in the wrong ID key for some reason. The key point is establishing who redeemed/burned those tokens; we can see which address it came from, but we don't know who should get the cash transferred into their bank account.

If the bank, added me as their customer and I held those tokens, then they already know my trusted circle-approved address, and they could pay me out based on that the tokens from THAT address got burned. It is only if bought my tokens on the exchange from someone else, and I am a customer of another bank in the trusted circle. Then they would ask which bank I am using and contact them and get my address verified.

The other low-tech alternative approach would be to arrange for me to make a new transfer from the address that redeemed the tokens to any address, my own for that matter. The important part is that I can add information to the transaction that you gave me. If I can sign it, then I control that address.

The financial institution could just say: please transfer 0.0001 TGD to this address tgrade1fuf0md08m0m2e0ummqkmkpqdfjfp57gy52mjd3 and put this string "fR02-Yxc5-2i28-ad36-HF3d-GFTD" in the memo field. If I fuck it up again, we can try one more try. After 10 or 20 attempts where it is the right ID but send from the wrong address, or send from the right address with the wrong ID, they would conclude that I cannot or will not prove to you that I control the address that burned the redeem tokens.

I like both the tech-alternative solutions, but let's make a simple first version, roll it out and get some feedback from the financial institutions. Let's then based on their input figure out what we do next.

ethanfrey commented 3 years ago

This response makes sense Kain. An issue (mentioned on Discord, but sticking here for posterity) is if the Redeem was called by an AMM to withdraw. In this case, the "sender" is the AMM not the original owner.

We could say "we don't care", or we could create a RedeemFor method that works just like redeem, but takes one more argument "original sender" that will be emitted in the event instead of the account sending the message, so the AMM could redeem the tokens it holds with my code, but mark my account as the "original sender" which can be contacted somehow if the code was wrong.

Actually, we could just add redeemer: Option<String> to the ExecuteMsg::Redeem{} type, which is left unfilled if I call directly, but may be filled out with the "original sender" if an AMM or other DeFi contract is using this to allow me to redeem my tokens after I have been kicked out of the Trusted Circle

ethanfrey commented 3 years ago

Closes by #46