Incorrect Parameter Order

[0xSandyy]

Checklist

• [x] I have searched the existing issues and pull requests for duplicates.

Type of Issue

• [x] New vulnerability addition
• [ ] Feature request
• [ ] Update existing vulnerability

Description

Incorrect Parameter Order

Solidity does not support named parameters, and of course, the order in which function parameters are passed is crucial. Mistakes in parameter ordering, mainly in functions with numerous parameters, are easily made. Such errors can have severe unintended consequences, especially when they affect accounting-related operations.

This is an example finding from sherlock: https://github.com/sherlock-audit/2023-02-gmx-judging/issues/97

./PositionUtils.sol

function updateTotalBorrowing(
    PositionUtils.UpdatePositionParams memory params,
    uint256 nextPositionSizeInUsd,
    uint256 nextPositionBorrowingFactor
) internal {
    MarketUtils.updateTotalBorrowing(
        params.contracts.dataStore,
        params.market.marketToken,
        params.position.isLong(),
        params.position.borrowingFactor(),
        params.position.sizeInUsd(),
        nextPositionSizeInUsd,
        nextPositionBorrowingFactor
    );
}

./MarketUtils.sol

function updateTotalBorrowing(
    DataStore dataStore,
    address market,
    bool isLong,
    uint256 prevPositionSizeInUsd,
    uint256 prevPositionBorrowingFactor,
    uint256 nextPositionSizeInUsd,
    uint256 nextPositionBorrowingFactor
) external {
    uint256 totalBorrowing = getNextTotalBorrowing(
        dataStore,
        market,
        isLong,
        prevPositionSizeInUsd,
        prevPositionBorrowingFactor,
        nextPositionSizeInUsd,
        nextPositionBorrowingFactor
    );

    setTotalBorrowing(dataStore, market, isLong, totalBorrowing);
}

Here, updateTotalBorrowing() of PositionUtils.sol calls updateTotalBorrowing() of MarketUtils.sol but you can see borrowingFactor and sizeInUsd values are interchanged.

Sources:

• https://github.com/sherlock-audit/2023-02-gmx-judging/issues/97

[kadenzipfel]

My concern with this is that it's general to all programming languages/paradigms so not sure if it fits in here. Will leave this open for now to see what others think

[0xSandyy]

Hi, it seems I made an error regarding this issue. Solidity do supports named arguments as of now if you use { } to pass in arguments. The entirety of this issue was based on solidity not supporting named arguments unlike other languages that makes incorrect parameter order issue more prevalent but this is not the case. I don't think this issue requires further discussion anymore.

On the plus side, I think we should create a separate listing of Best Practices which includes developer recommendations and useful coding patterns for developing more secure and optimized smart contracts. For example: Using named parameters is one. Named parameters are available in solidity now but they are rarely used while developing smart contracts. What do you think of this? @kadenzipfel

[kadenzipfel]

On the plus side, I think we should create a separate listing of Best Practices which includes developer recommendations and useful coding patterns for developing more secure and optimized smart contracts. For example: Using named parameters is one. Named parameters are available in solidity now but they are rarely used while developing smart contracts. What do you think of this? @kadenzipfel

I've thought about this a bit, but not sure. Feel free to open an issue for discussion.