Reentrancy through `withdrawBounty`

[code423n4]

Handle

jonah1005

Vulnerability details

Impact

The function settleAuction Auction.sol#L69-L109 calls withdrawBounty. However, there's no safety checks in addBounty function.Auction.sol#L126-L138 The attacker can add malicious contract through addBounty and hijack the control flow of settleAuction through withdrawBounty.

The authetication checks located before reentrancy point.

    require(auctionOngoing);
    require(hasBonded);
    require(bondTimestamp + ONE_DAY > block.number);
    require(msg.sender == auctionBonder);

The attacker can enter settleAuction twice. I: -> calls settleAuction -> pass authentication checks -> enter II in withdrawBounty

II: -> calls settleAuction again but with normal parameters -> the auction.auctionOngoing() is now return false. Attacker can mint token in the basket contract -> calls mint in the basket. This would call handleFees and make ibRate smaller than the current Ibrate -> mint large amount of basket tokens. -> return to I

I: -> the auction contract calls basket.updateIBRatio and set ibRate back to the newRate (original one). -> burn all basket tokens at a higher ibRate. -> Since basket.lastFee is set when II calls mint. The ibRate would not change this time.

        uint256 a = factory.auctionMultiplier() * basket.ibRatio();
        uint256 b = (bondTimestamp - auctionStart) * BASE / factory.auctionDecrement();
        uint256 newRatio = a - b;

The attacker could possibly drain all the tokens in the basket by launching a flashloan attack.

I consider this is a high-risk issue.

Proof of Concept

As stated above.

Tools Used

None

Recommended Mitigation Steps

There are two issues involved.

1. addBounty should not accept whatever tokens. A possible fix is to add a list that only DAO can decide which kind of tokens are accepted.
2. settleAuction should add a nonReentrant modifier. settleAuction is a sensitive function and should treat it with extra caution. (ref: https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard)

An attacker can pass whatever inputTokens and outputTokens to settleAuction. While it seems to be safe as the reentrancy point located before the balance check. require(IERC20(pendingTokens[i]).balanceOf(address(basket)) >= tokensNeeded); It's against the best practice. Recommend to fix it as well.

[frank-beard]

duplicate of https://github.com/code-423n4/2021-09-defiprotocol-findings/issues/31

[GalloDaSballo]

Duplicate of #31