hats-finance / Thorn-protocol-0x1286ecdac50215a366458a14968fbca4bd95067d

GNU General Public License v3.0
0 stars 0 forks source link

`StableSwapRouter.sol` contract have confidentiality issue which is against the oasis sapphire system. #9

Open hats-bug-reporter[bot] opened 1 month ago

hats-bug-reporter[bot] commented 1 month ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x8ad838cddb24845cde5155d7405180244780c021a2eb865b002ce1c38d8223d7 Severity: medium

Description: the contract StableSwapRouter.sol have confidentiality issue which is against the oasis sapphire docs

the sapphire docs requires that the events with confidential information of users should not be emitted otherwise there is no point in using oasis sapphire cause there is no confidentiality.

and in function exactInputStableSwap( we can see that the event

 emit StableExchange(msg.sender,amountIn,path[0],amountOut,path[path.length - 1], to);

is emited

this event emits msg.sender address in event of stable exchange it loses its confidentiality and is a RISK in ecosystem.

oasis docs https://docs.oasis.io/dapp/sapphire/guide#contract-logs this explains the case in erc20 structure but the point is same, saving confidentiality

Contract Logs

Contract logs/events (e.g., those emitted by the Solidity emit keyword) are exactly like Ethereum. Data contained in events is not encrypted. Precompiled contracts are available to help you encrypt data that you can then pack into an event, however. Unmodified contracts may leak state through logs Base contracts like those provided by OpenZeppelin often emit logs containing private information. If you don't know they're doing that, you might undermine the confidentiality of your state. As a concrete example, the ERC-20 spec requires implementers to emit an event Transfer(from, to, amount), which is obviously problematic if you're writing a confidential token. What you can do instead is fork that contract and remove the offending emissions.

Recommendation/

.

Ghoulouis commented 1 month ago

In this first version, we aim for a stable and secure version with tokens on the Sapphire network, privacy functionality will be implemented later when everything is in orbit. Hiding events makes little sense when the 3rd party ERC-20 contracts also emit events, thanks for your report.

0xarshia commented 1 month ago

Thanks for the careful review

However i think there is issue

This issue was not disclosed in known issues in contests readme page which would make this issue invalid by rules

And you accept the fact that this change is necessary

Then respectfully i don't see enough reason to make this issue Invalid

Cause it would then make the readme page not source of truth and trust

And yes as erc20 would make issue too the doc i mentioned says that too you need to create fork of them if you want to save privacy to do private swaps

But this doesn't change the fact that you are going to change code either now or in future doesn't matter

Therefore I believe this issue is valid Thanks

omega-audits commented 1 month ago

Confidentially is a feature of Oasis, but in no way a requirement. What you are describing as a bug here is really just a desirable feature. The developers states that clearly in his answer: they are intending to add this in a future version, but not as of now, as thre is no urgency to "fix" this.

0xarshia commented 1 month ago

Thanks for your comment and percise reviews

its good to see protocol team claim that they need to fix this problem sooner or later but they will fix it

however from the way hats finance operates, if issue is going to fix then its also eligible for payout, as it made clear multiple times by hats team.

and because it was not mentioned in any part of readme page as "known issue" its completely falls to the category of valid issues

about this being feature or not, i have to say Oasis Sapphire built with the only main purpose of confidentiality and its literally the only reason why this protocol is private swap and made on sapphire we cannot say its some sort of "Feature" its actually a key factor in sapphire and A reason why sapphire built for.

i believe discussing that this issue being feature or not is not correct at all.

at the end of the day oasis sapphire support projects being built on sapphire because of contributing to ecosystem with confidentiality.

as in Thorn protocol docs confidentiality is mentioned multiple times with target of "MAXIMIZING THE Confidentiality" as being one of the number one important goal of the protocol and that users need to be more vigilant that they don't leak private info accidently, so we cannot deny that its key factor in project

and as protocol team said they also aim to fix it,

so i believe issue is valid by the impact and hats rules

Thanks