code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/coinswap/keeper/keeper.go#L176-L178

Vulnerability details

Impact

The documentation states that:

There is a limit on the number of Canto tokens for each pool.
Deposits will fail if the amount of Canto for the pool exceeds 10,000 Canto.

This holds true only in case of using the AddLiquidity message of the coinswap module. A malicious user can send funds directly to the pool's address, bypassing the limit. In that case the attacker will not receive the LP tokens though and will not be able to remove the liquidity later on. The pool address is known and is not considered a module address and therefore not blocked from transfers. The pool address can be calculated by:

func GetReservePoolAddr(lptDenom string) sdk.AccAddress {
    return sdk.AccAddress(crypto.AddressHash([]byte(lptDenom)))
}

Or through a query request to the coinswap module.

While this doesn't have any direct effects except for pool price manipulation, the project did go a long way to implement a mechanism to limit the deposits. Therefore I consider this as medium issue.

Proof of Concept

Just send funds directly to the pool address.

Tools Used

IDE.

Recommended Mitigation Steps

One possible solution, that's easy to implement, is to add the three whitelisted pool addresses to the blocked addresses list. This is a valid solution as long as the pools are permissioned, because they're known beforehand.

Another possible solution, which is harder to implement, is to change the pools to only use the funds added directly by AddLiquidity and ignore the balance that was added by direct sends.

Assessed type

Token-Transfer

c4-pre-sort commented 1 year ago

JeffCX marked the issue as primary issue

tkkwon1998 commented 1 year ago

I think dupe of #9

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)