search

sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

0xmuxyz - Lack of access control modifier on the Product#`closeTakeFor()` and the Product#`closeMakeFor()`, which allow a malicious user to be freely able to close any existing user's Maker/Taker position #171

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

0xmuxyz

high

Lack of access control modifier on the Product#closeTakeFor() and the Product#closeMakeFor(), which allow a malicious user to be freely able to close any existing user's Maker/Taker position

Summary

Lack of access control modifier on the Product#closeTakeFor() and the Product#closeMakeFor(), which allow a malicious user to be freely able to close any existing user's Maker/Taker position.

Vulnerability Detail

When a taker position would be closed, the Product#closeTake() would be called. Within the Product#closeTake(), the Product#closeTakeFor() would be called like this: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L238

    /**
     * @notice Closes a taker position for `msg.sender`
     * @param amount Amount of the position to close
     */
    function closeTake(UFixed18 amount) external {
        closeTakeFor(msg.sender, amount);  /// @audit
    }

Within the Product#closeTakeFor(), the Product#_closeTake() would be called in order to close the amount of the taker position that the account has like this: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L255

    /**
     * @notice Closes a taker position for `account`. Deducts position fee based on notional value at `latestVersion`
     * @param account Account to close the position for
     * @param amount Amount of the position to close
     */
    function closeTakeFor(address account, UFixed18 amount)
        public  /// @audit
        nonReentrant
        notPaused
        onlyAccountOrMultiInvoker(account)
        settleForAccount(account)
        closeInvariant(account)
        liquidationInvariant(account)
    {
        _closeTake(account, amount);  /// @audit 
    }

Within the Product#_closeTake(), amount of the taker position that the account has would be closed like this: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L258-L272

    function _closeTake(address account, UFixed18 amount) private {
        IOracleProvider.OracleVersion memory latestOracleVersion = atVersion(latestVersion());

        _positions[account].pre.closeTake(latestOracleVersion.version, amount);
        _position.pre.closeTake(latestOracleVersion.version, amount);

        UFixed18 positionFee = amount.mul(latestOracleVersion.price.abs()).mul(takerFee());
        if (!positionFee.isZero()) {
            controller().collateral().settleAccount(account, Fixed18Lib.from(-1, positionFee));
            emit PositionFeeCharged(account, latestOracleVersion.version, positionFee);
        }

        emit PositionFeeCharged(account, latestOracleVersion.version, positionFee);
        emit TakeClosed(account, latestOracleVersion.version, amount);
    }

Also, when a maker position would be closed, the Product#closeMake() would be called. Within the Product#closeMake(), the Product#closeMakeFor() would be called like this: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L320

    /**
     * @notice Closes a maker position for `msg.sender`
     * @param amount Amount of the position to close
     */
    function closeMake(UFixed18 amount) external {
        closeMakeFor(msg.sender, amount);  /// @audit
    }

Within the Product#closeMakeFor(), the Product#_closeMake() would be called in order to close the amount of the maker position that the account like this: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L338

    /**
     * @notice Closes a maker position for `account`. Deducts position fee based on notional value at `latestVersion`
     * @param account Account to close the position for
     * @param amount Amount of the position to close
     */
    function closeMakeFor(address account, UFixed18 amount)
        public  /// @audit
        nonReentrant
        notPaused
        onlyAccountOrMultiInvoker(account)
        settleForAccount(account)
        takerInvariant
        closeInvariant(account)
        liquidationInvariant(account)
    {
        _closeMake(account, amount); /// @audit
    }

Within the Product#_closeMake(), amount of the maker position that the account has would be closed like this: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L338

    function _closeMake(address account, UFixed18 amount) private {
        IOracleProvider.OracleVersion memory latestOracleVersion = atVersion(latestVersion());

        _positions[account].pre.closeMake(latestOracleVersion.version, amount);
        _position.pre.closeMake(latestOracleVersion.version, amount);

        UFixed18 positionFee = amount.mul(latestOracleVersion.price.abs()).mul(makerFee());
        if (!positionFee.isZero()) {
            controller().collateral().settleAccount(account, Fixed18Lib.from(-1, positionFee));
            emit PositionFeeCharged(account, latestOracleVersion.version, positionFee);
        }

        emit PositionFeeCharged(account, latestOracleVersion.version, positionFee);
        emit MakeClosed(account, latestOracleVersion.version, amount);
    }

The functions to close the taker/maker position is supposed to be called by the position owner. Also, only a taker/maker position of the caller (msg.sender) is supposed to be able to be closed.

However, the Product#closeTakeFor() and the Product#closeMakeFor() above can be called by any user (including a malicious user) because of that the access control modifier on both functions would be "public".

Therefore, any user can call the the Product#closeTakeFor() or the Product#closeMakeFor() and assign any existing address into the account parameter of them. This allow a malicious user to be freely able to close any existing user's taker/maker position.

Impact

This allow a malicious user to be freely able to close any existing user's taker/maker position.

Code Snippet

Tool used

Manual Review

Recommendation

Consider changing the access control modifier on the Product#closeTakeFor() and the Product#closeMakeFor()from public to internal.

    function closeTakeFor(address account, UFixed18 amount)
+       internal
-       public
        nonReentrant
        notPaused
        onlyAccountOrMultiInvoker(account)
        settleForAccount(account)
        closeInvariant(account)
        liquidationInvariant(account)
    {
        _closeTake(account, amount); 
    }
    function closeMakeFor(address account, UFixed18 amount)
+       internal
-       public
        nonReentrant
        notPaused
        onlyAccountOrMultiInvoker(account)
        settleForAccount(account)
        takerInvariant
        closeInvariant(account)
        liquidationInvariant(account)
    {
        _closeMake(account, amount);
    }