Kuiper contest - jonah1005's results

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.

Handle

jonah1005

Vulnerability details

Impact

The aution contract decides a new ibRatio in the function settleAuction. Auction.sol#L89-L91

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

There's a chance that newRatio would be really close to zero. This imposes too much risk on the protocol. The network may not really be healthy all the time. Solana and Arbitrum were down and Ethereum was suffered a forking issue recently. Also, the network may be jammed from time to time. This could cause huge damage to a protocol. Please refer to Black Thursday for makerdao 8.32 million was liquidated for 0 dai

Given the chance that all user may lose their money, I consider this is a medium-risk issue.

Proof of Concept

Black Thursfay for makerdao 8.32 million was liquidated for 0 dai bug-impacting-over-50-of-ethereum-clients-leads-to-fork

Tools Used

None

Recommended Mitigation Steps

I recommend setting a minimum ibRatio when a publisher publishes a new index. The auction should be killed if the ibRatio is too low.

Handle

jonah1005

Vulnerability details

Impact

The aution contract decides a new ibRatio in the function settleAuction. Auction.sol#L89-L91

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

In this equation, a would not always be greater than b. The auctionBonder may lock the token in bondForRebalance() at a point that a-b would always revert.

The contract should not allow users to lock the token at the point that not gonna succeed. Given the possible (huge) loss of the user may suffer, I consider this is a medium-risk issue.

Proof of Concept

Here's a web3.py script to trigger this bug.

basket.functions.publishNewIndex([dai.address], [deposit_amount]).transact()

for i in range(4 * 60 * 24):
    w3.provider.make_request('evm_mine', [])
basket.functions.publishNewIndex([dai.address], [deposit_amount]).transact()

print('auction on going', auction.functions.auctionOngoing().call())
for i in range(20000):
    w3.provider.make_request('evm_mine', [])

all_token = basket.functions.balanceOf(user).call()
basket.functions.approve(auction.address, all_token).transact()
auction.functions.bondForRebalance().transact()
# error Log
# {'code': -32603, 'message': 'Error: VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)'}
auction.functions.settleAuction([], [], [], [], []).transact()

Tools Used

None

Recommended Mitigation Steps

Recommend to calculate the new irate in bondForRebalance. I understand the auctionBonder should take the risk to get the profit. However, the contract should protect the user in the first place when this auction is doomed to fail.

Handle

jonah1005

Vulnerability details

Impact

Basket.sol#L224-L228 calls approve that do not handle non-standard erc20 behavior.

1. Some token contracts do not return any value.
2. Some token contracts revert the transaction when the allowance is not zero.

Since the auction contract calls setNewWeights in function settleAuction, auctionBonder may lock its bond and never successfully settles the auction. This leads to the auctionBonder loss he's bond and the basket and the auction becomes suspended.

auctionBonder would lose his bond and the contract would be suspended. I consider this a high-risk issue.

Proof of Concept

USDT may be a classic non-standard erc20 token.

Here's a short POC.

usdt.functions.approve(basket.address, 100).transact()
## the second tx would be reverted as the allowance is not zero
usdt.functions.approve(basket.address, 50).transact()

Tools Used

None

Recommended Mitigation Steps

Recommend to use safeApprove instead and set the allowance to 0 before calling it.

    function approveUnderlying(address spender) private {
        for (uint256 i = 0; i < weights.length; i++) {
            IERC20(tokens[i]).safeApprove(spender, 0);
            IERC20(tokens[i]).safeApprove(spender, type(uint256).max);
        }
    }