Unexpected return values for some sets of parameters passed into _computeAssetAmounts

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L222

Vulnerability details

Impact

Detailed description of the impact of this finding.

_computeAssetAmounts() may not be able to handle smaller parameter values. The function's return value uint256 feeAmount may be zero. Although this is return value is checked consistenly after making an internal call to the function, this may mean that small amounts of uint256 assets do not have fees deducted, resulting in a loss of potential fee revenue.

This is due to rounding down in Solidity when the result of division is a non-integer value. For example, iF fees[f] = 5,000 and assets = 20, the result will be feeAmount = (assets f) / FEE_DENOMINATOR = (20 5000) / 1_000_000 = 100_000 / 1_000_000 = 0 due to rounding down. This will pass the assert statement after the calculation is completed (20 + 0 == 20).

All values of (asset*f) that are less than the FEE_DENOMINATOR will return zero. If fees are changed, the amount of assets that can be handled with no fee will also change.

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.17;

import "../lib/forge-std/src/Test.sol";

contract Rounding {
    uint256 public constant FEE_DENOMINATOR = 1_000_000;

    function _computeAssetAmounts(uint256 f, uint256 assets)
        public
        view
        returns (uint256 postFeeAmount, uint256 feeAmount)
    {
        feeAmount = (assets * f) / FEE_DENOMINATOR;
        postFeeAmount = assets - feeAmount;

        assert(feeAmount + postFeeAmount == assets);
    }
}

contract rounding is Test {
    Rounding public Contract;

    function setUp() public {
        Contract = new Rounding();
    }

    function testRounding() public {
        (uint256 postFeeAmount, uint256 feeAmount) = Contract
            ._computeAssetAmounts(5000, 20);

        assertEq(feeAmount, 0);
    }
}

Running 1 test for test/Counter.t.sol:rounding
[PASS] testRounding() (gas: 5926)
Test result: ok. 1 passed; 0 failed; finished in 500.18µs

Tools Used

manual review, foundry

Recommended Mitigation Steps

Set a minimum amount of assets that can be handled. require(assets*fees[f] >= FEE_DENOMINATOR)

[Picodes]

Fees being rounded down is the desired behavior, and the 0 fee will only concern unsignificant amounts.

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-11-redactedcartel-findings/issues/358