sherlock-audit / 2023-12-jojo-exchange-update-judging

10 stars 6 forks source link

Varun_05 - Lack of slippage check when a user requests a withdraw in FundingRateArbitrage.sol #71

Closed sherlock-admin2 closed 9 months ago

sherlock-admin2 commented 10 months ago

Varun_05

medium

Lack of slippage check when a user requests a withdraw in FundingRateArbitrage.sol

Summary

When a user requests a withdraw and when the withdrawal requests are permitted there can be decrease in the value of getIndex() which can cause a user to loss.

Vulnerability Detail

Following is request withdraw function

 function requestWithdraw(uint256 repayJUSDAmount) external returns (uint256 withdrawEarnUSDCAmount) {
        IERC20(jusd).safeTransferFrom(msg.sender, address(this), repayJUSDAmount);
        require(repayJUSDAmount <= jusdOutside[msg.sender], "Request Withdraw too big");
        jusdOutside[msg.sender] -= repayJUSDAmount;
        uint256 index = getIndex();
        uint256 lockedEarnUSDCAmount = jusdOutside[msg.sender].decimalDiv(index);
        require(
            earnUSDCBalance[msg.sender] >= lockedEarnUSDCAmount, "lockedEarnUSDCAmount is bigger than earnUSDCBalance"
        );
        withdrawEarnUSDCAmount = earnUSDCBalance[msg.sender] - lockedEarnUSDCAmount;
        withdrawalRequests.push(WithdrawalRequest(withdrawEarnUSDCAmount, msg.sender, false));
        require(
            withdrawEarnUSDCAmount.decimalMul(index) >= withdrawSettleFee, "Withdraw amount is smaller than settleFee"
        );
        earnUSDCBalance[msg.sender] = lockedEarnUSDCAmount;
        uint256 withdrawIndex = withdrawalRequests.length - 1;
        emit RequestWithdrawFromHedging(msg.sender, repayJUSDAmount, withdrawEarnUSDCAmount, withdrawIndex);
        return withdrawIndex;
    }

and following is permit withdraw requests

 function permitWithdrawRequests(uint256[] memory requestIDList) external onlyOwner {
        uint256 index = getIndex();
        for (uint256 i; i < requestIDList.length; i++) {
            WithdrawalRequest storage request = withdrawalRequests[requestIDList[i]];
            require(!request.isExecuted, "request has been executed");
            uint256 USDCAmount = request.earnUSDCAmount.decimalMul(index);
            require(USDCAmount >= withdrawSettleFee, "USDCAmount need to bigger than withdrawSettleFee");
            uint256 feeAmount = (USDCAmount - withdrawSettleFee).decimalMul(withdrawFeeRate) + withdrawSettleFee;
            if (feeAmount > 0) {
                IERC20(usdc).transfer(owner(), feeAmount);
            }
            IERC20(usdc).transfer(request.user, USDCAmount - feeAmount);
            request.isExecuted = true;
            totalEarnUSDCBalance -= request.earnUSDCAmount;
            emit PermitWithdraw(request.user, USDCAmount, feeAmount, request.earnUSDCAmount, requestIDList[i]);
        }
    }

If in request withdraw function index is high then it can cause the value of the following to be very less

withdrawEarnUSDCAmount = earnUSDCBalance[msg.sender] - lockedEarnUSDCAmount;

But when the request is fulfilled then index can be low due to which when the following is done

uint256 USDCAmount = request.earnUSDCAmount.decimalMul(index);

if index is low it further reduces the value of USDCAmount and it can also cause USDCAmount to be equal to feeAmount and thus no usdc would be transferred to the user. A scenario where index can reduce is lot of users request withdrawls or when jusd is borrowed after calculating index in the request withdraw function which decreases the net value but the total earn usdc amount remains the same

function getNetValue() public view returns (uint256) {
        uint256 jusdBorrowed = IJUSDBank(jusdBank).getBorrowBalance(address(this));
        uint256 collateralAmount = IJUSDBank(jusdBank).getDepositBalance(collateral, address(this));
        uint256 usdcBuffer = IERC20(usdc).balanceOf(address(this));
        uint256 collateralPrice = IJUSDBank(jusdBank).getCollateralPrice(collateral);
        (int256 perpNetValue,,,) = JOJODealer(jojoDealer).getTraderRisk(address(this));
        return
  @===>      SafeCast.toUint256(perpNetValue) + collateralAmount.decimalMul(collateralPrice) + usdcBuffer - jusdBorrowed;(<===)
    }

The above is valid because if there is large amount of withdrawls requests then all of them can not be executed in a single transaction.

Impact

Causes a loss to the users as they might not receive any usdc.

Code Snippet

https://github.com/sherlock-audit/2023-12-jojo-exchange-update/blob/main/smart-contract-EVM/src/FundingRateArbitrage.sol#L304

Tool used

Manual Review

Recommendation

Add a input in withdraw requests where the user defines the minimum value they want to receive.

Duplicate of #35

sherlock-admin2 commented 9 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid because { This is valid and a dupp of 066; but the recommendation should be discarded and instead use the same amount or index used during requestWithdraw instead of recalculating it}