GrosQuildu
commented
1 year ago The "burns transferred coins in the source chain" statement is misleading, but one may argue that it is somehow valid, as per the cosmos comment:
The coins (vouchers) are burned on the sender chain and then transferred to the receiving chain though IBC
What you wrote is much more clear and precise, and we should clarify the "IBC" exploit scenario. To be more precise we should consider four cases: when the chain with AMM
- is sending coins to other chain - coins are escrowed
- is receiving coins from other chain - coins are minted
- is sending coins back to original chain - coins are burned
- is receiving coins back - coins are taken from escrowed address
1 and 4 shouldn't impact total supply (uTokensInCirculation) - no issue here (as you noticed). Spendable coins amount (tokensHeld) may be impacted though, because escrowing is locking coins - but from the code it seems like there is no real "lock" like it is in with delegating coins (?)
2 and 3 change total supply. However, I don't remember how coin's prefixes influence these computations. At the best case the AMM is not buggy, because burning and minting is of other coins (with a prefix, different denomination). Not sure.
We should rewrite the "IBC" exploit scenario to be more generic and less specific (e.g., "be aware of IBC transfers that may influence balances"). Ideally, we research this topic more and provide accurate and tested "IBC" exploit scenario. It was some time ago when I wrote the issue, and now don't have testing code around anymore. Also the IBC code may have changed.
If you are willing to research the topic, we would gladly accept a PR.
The list of Cosmos related findings is super awesome! It's the only place I've been able to find a good list of Cosmos blockchain security issues. So, thanks for doing that. @oldsj
Before I say anything, I'm fine being wrong but just want to make sure I fully understand this. On the finding broken_bookkeeping, one of the ways mentioned to remove the funds to mess with the total supply of the AMM is the usage of IBC. This claims to work because moving coins from one chain to another burns the supply.
However, my understanding is that tokens on the source chain are escrowed within a module specific account. If it's a non-source chain, then they are burned. The code snippet in IBC go that is linked exists within the non-source chain path, which means that the tokens are burned. The source chain path appears to escrow the funds. If the tokens for the AMM are from an IBC chain, this would absolutely be an issue though. I don't have the full context of the project that this report was built on though so it's hard for me to say.
Am I misunderstanding something here? I just want to make sure that this is documented correctly for myself and everyone else who is using this repository.