code-423n4 / 2023-07-tapioca-findings

15 stars 10 forks source link

Lack of slippage checks on public withdraw fees function #1661

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L232-L248

Vulnerability details

Impact

    function withdrawAllMarketFees(
        IMarket[] calldata markets_,
        ISwapper[] calldata swappers_,
        IPenrose.SwapData[] calldata swapData_
    ) public notPaused {
        require(
            markets_.length == swappers_.length &&
                swappers_.length == swapData_.length,
            "Penrose: length mismatch"
        );
        require(address(swappers_[0]) != address(0), "Penrose: zero address");
        require(address(markets_[0]) != address(0), "Penrose: zero address");

        _withdrawAllProtocolFees(swappers_, swapData_, markets_);

        emit ProtocolWithdrawal(markets_, block.timestamp);
    }

Here, protocol checks for valid allowed swappers down in call context with

        require(swappers[swapper], "Penrose: Invalid swapper");

but does not have checks for _swapData, a malicious user can sandiwtch by calling withdrawAllMarketFees with zero slippage swap data

Proof of Concept

N/A (covered in above section)

Tools Used

Manual Review

Recommended Mitigation Steps

Use fixed slippage threshold or make it gated function

Assessed type

Access Control

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #266

c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #245

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Insufficient proof