Attacker can manipulate the usds supply

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L115

Vulnerability details

When a user repays their borrow, the USDS tokens used for repayment are sent to the USDS contract. Subsequently, these tokens are burned directly in the USDS contract by invoking the burnTokensInContract function. This function is accessible to any user or the UpKeep contract."

 function burnTokensInContract() external
        {
        uint256 balance = balanceOf( address(this) );
        _burn( address(this), balance );

        emit USDSTokensBurned(balance);
        }

[Link]

These tokens are not burned instantly but are burned later by the UpKeep contract (potentially through calls made by other users in the usds contract).

This behavior can be exploited if an attacker creates a smart contract that iteratively borrows and repays the same maximum amount. This repetitive cycle could lead to a significant increase in the supply of USDS tokens.

Impact

The impact of this finding may not be directly exploitable within the Salty protocol. However, it poses a potential threat to other protocols that is going to rely on the USDS totalSupply, such as vaults, certain decentralized exchanges, or lending platforms. This becomes a significant concern as it deviates from the typical behavior of an ERC20 token.

Proof of Concept

Run the next proof of concept in the file: src/stable/tests /CollateralAndLiquidity.t.sol

function testIncreaseSuply() public {//@note
        console.log(usds.totalSupply());

        //deposit collateral and borrow the max usds possible
        _depositCollateralAndBorrowMax(alice);

        vm.startPrank(alice);
        //repaying the first time the borrow usds
        collateralAndLiquidity.repayUSDS(collateralAndLiquidity.usdsBorrowedByUsers(alice));

        //borrow and repay so many times in a loop increasing the total supply  to much
        for (uint256 i; i < 100; i++) {
            uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
            collateralAndLiquidity.borrowUSDS(maxUSDS);
            collateralAndLiquidity.repayUSDS(collateralAndLiquidity.usdsBorrowedByUsers(alice));
        }
        vm.stopPrank();

        console.log(usds.totalSupply());
    }

See the logs:

Total supply at the beggining: 2000000000000000000000000

Total supply at the final: 238802436875000000000000000000

Tools Used

Manual, Foundry

Recommended Mitigation Steps

Consider burn the usds instanly when a user repay his borrow and not increment the usdsThatShouldBeBurned variable.

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[Picodes]

Related to #618

[c4-sponsor]

othernet-global (sponsor) acknowledged

[Picodes]

I'll consider that this is a dup of #618

[c4-judge]

Picodes marked the issue as duplicate of #137

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[jorgect207]

Hey @Picodes, thank you so much for your time.

I think that my issue is not a duplicate of the #137 and its duplicates, and here's why:

1. Different impacts: Most of the impacts are related to draining the liquidity from the DAO, or depegging to the USD value. My impact is directly damaging to the USDS token, and has serious problems and flags if the token is going to be integrated with DeFi protocols (DEXes, lending platforms, vaults, among others).
2. The recommendation also doesn't solve the issue, as sending the tokens to the liquidizer doesn't decrease the supply of the USDC token.

[c4-judge]

Picodes marked the issue as not a duplicate

[Picodes]

@jorgect207 thank you for flagging.

Indeed this isn't a duplicate of #137. This report shows how the total supply could be temporarily inflated. It has no impact within this contest and would be an issue for integrators only if they make incorrect assumptions about the token. The ERC20 standard isn't broken. So I'll downgrade to Low.

Side note: it is quite standard to have "inflated" total supplies, especially among stablecoins where the total supply is a KPI. For example, you could check EURT https://etherscan.io/token/0xC581b735A1688071A1746c968e0798D642EDE491?a=0x5754284f345afc66a98fbb0a0afe71e0f007b949 where 90% of the tokens are held by the treasury.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)