code-423n4 / 2024-04-panoptic-findings

7 stars 3 forks source link

Missing deadline checks in CollateralTracker.sol crucial functions like the `deposit()` and `withdraw()` #493

Closed c4-bot-9 closed 4 months ago

c4-bot-9 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L417 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L531 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L477 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L591

Vulnerability details

In CollateralTracker.sol the

[withdraw()](https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L531)
```solidity
    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) external returns (uint256 shares) {
        if (assets > maxWithdraw(owner)) revert Errors.ExceedsMaximumRedemption();

        shares = previewWithdraw(assets);

        // check/update allowance for approved withdraw
        if (msg.sender != owner) {
            uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

            if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares;
        }

        // burn collateral shares of the Panoptic Pool funds (this ERC20 token)
        _burn(owner, shares);

        // update tracked asset balance
        unchecked {
            s_poolAssets -= uint128(assets);
        }

        // transfer assets (underlying token funds) from the PanopticPool to the LP
        SafeTransferLib.safeTransferFrom(
            s_underlyingToken,
            address(s_panopticPool),
            receiver,
            assets
        );

        emit Withdraw(msg.sender, receiver, owner, assets, shares);

        return shares;
    }

Impact

User transactions may be delayed for too long and executed at an unfavourable time both intentionally or by coincidence leading to unwanted effects to the protocol users

Tools Used

Manual Review

Recommended Mitigation Steps

A deadline check should be added to these crucial functions.

Assessed type

Other

c4-judge commented 4 months ago

Picodes marked the issue as duplicate of #365

c4-judge commented 4 months ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 4 months ago

Picodes marked the issue as grade-c