Stableswap pool is incompatible with some common stable coin(e.g. USDT) with an optional fee feature, and will have erroneous reserve accounting

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L1072-L1073 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L752-L753 https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L825-L826

Vulnerability details

Impact

Stableswap pool is incompatible with some common stable coin(e.g. USDT) with an optional fee feature, and will have erroneous reserve accounting

Proof of Concept

Some common stable coins such as USDT might have a fee feature that can be optionally turned on. Stableswap pool will not handle this correctly in asset reserve accounting.

The vulnerability is in handling asset transfer, the before and after balance of a target asset needs to be checked to correctly handle a potential fee deduction mechanism of the asset.

Current transfer mechanism in do_add_liqudity(), sell() and buy() will assume the amount input in T::Currency::transfer() will be the balance different, failing to account for potential fees.

//HydraDX-node/pallets/stableswap/src/lib.rs
    fn do_add_liquidity(
        who: &T::AccountId,
        pool_id: T::AssetId,
        assets: &[AssetAmount<T::AssetId>],
    ) -> Result<Balance, DispatchError> {
...
        T::Currency::deposit(pool_id, who, share_amount)?;
                //@audit this assume user input asset.amount will be the balance difference of the pool, this will be incorrect when token transfer fee(e.g. USDT) is enabled 
        for asset in assets.iter() {
|>          T::Currency::transfer(asset.asset_id, who, &pool_account, asset.amount)?;
        }

(https://github.com/code-423n4/2024-02-hydradx/blob/603187123a20e0cb8a7ea85c6a6d718429caad8d/HydraDX-node/pallets/stableswap/src/lib.rs#L1070-L1073)

Tools Used

Manual

Recommended Mitigation Steps

Consider referencing curve's StableSwap3Pool implementation to handle a potential fee token. Flag fee_index for a token with an optional fee feature, and when transferring a fee_index token, check the balance before and after transfer.

Assessed type

Token-Transfer

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[c4-sponsor]

enthusiastmartin (sponsor) disputed

[enthusiastmartin]

we dont support token with such features. all tokens present on hydra are represented by simple u32 integer. No token has additional features

[OpenCoreCH]

This finding does not apply to Polkadot / the HydraDX codebase because unlike Ethereum, assets are standardized. For instance, the mentioned USDT is just asset ID 1984 and cannot enforce a fee on transfer: https://assethub-polkadot.subscan.io/assets/1984?tab=holders

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid