code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/policies/Operator.sol#L330 https://github.com/code-423n4/2022-08-olympus/blob/2a0b515012b4a40076f6eac487f7816aafb8724a/src/modules/TRSRY.sol#L109-L112

Vulnerability details

Impact

In Operator.swap(), it assumes the reserve token is not a fee-on-transfer token and calculates amountOut from amountIn_ directly here.

But there is no garrantee that reserve token is a normal token and real received amount might be less than amountIn_ after this transfer.

As a result, a user might receive more ohm token then he should here.

Proof of Concept

As we can see here, TRSRY contract is designed to handle non-standard tokens originally.

We should modify other parts to manage such tokens properly also.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We can modify this part like below.

} else if (tokenIn_ == reserve) {
    /// Revert if upper wall is inactive
    if (!RANGE.active(true)) revert Operator_WallDown();

    /// Transfer reserves to treasury
    uint256 prevBalance = reserve.balanceOf(address(TRSRY)); //+++++++++++++++++++++++++++++++
    reserve.safeTransferFrom(msg.sender, address(TRSRY), amountIn_); //+++++++++++++++++++++++++++++++
    uint256 received = reserve.balanceOf(address(TRSRY)) - prevBalance; //+++++++++++++++++++++++++++++++

    /// Calculate amount out (checks for sufficient capacity)
    amountOut = getAmountOut(tokenIn_, received); //+++++++++++++++++++++++++++++++

    /// Revert if amount out less than the minimum specified
    /// @dev even though price is fixed most of the time,
    /// it is possible that the amount out could change on a sender
    /// due to the wall prices being updated before their transaction is processed.
    /// This would be the equivalent of the heart.beat front-running the sender.
    if (amountOut < minAmountOut_)
        revert Operator_AmountLessThanMinimum(amountOut, minAmountOut_);

    /// Decrement wall capacity
    _updateCapacity(true, amountOut);

    /// If wall is down after swap, deactive the cushion as well
    _checkCushion(true);

    /// Mint OHM to sender
    MINTR.mintOhm(msg.sender, amountOut);

    emit Swap(reserve, ohm, amountIn_, amountOut);
} else {

Oighty commented 2 years ago

There is no FOT check because the reserve is a whitelisted token by the protocol.

0xean commented 2 years ago

closing as invalid.

code-423n4 / 2022-08-olympus-findings