Kuiper contest - kenzo's results

Handle

kenzo

Vulnerability details

The settleAuction() function calls withdrawBounty() before setting auctionOngoing = false, thereby allowing reentrancy.

Impact

A malicious publisher can bypass the index timelock mechanism and publish new index which the basket's users won't have time to respond to. At worst case, this means setting weights that allow the publisher to withdraw all the basket's underlying funds for himself, under the guise of a valid new index.

Proof of Concept

1. The publisher (a contract) will propose new valid index and bond the auction. To settle the auction, the publisher will execute the following steps in the same transaction:
2. Add a bounty of an ERC20 contract with a malicious transfer() function.
3. Settle the valid new weights correctly (using settleAuction() with the correct parameters, and passing the malicious bounty id).
4. settleAuction() will call withdrawBounty() which upon transfer will call the publisher's malicious ERC20 contract.
5. The contract will call settleAuction() again, with empty parameters. Since the previous call's effects have already set all the requirements to be met, settleAuction() will finish correctly and call setNewWeights() which will set the new valid weights and set pendingWeights.pending = false.
6. Still inside the malicious ERC20 contract transfer function, the attacker will now call the basket's publishNewIndex(), with weights that will transfer all the funds to him upon his burning of shares. This call will succeed to set new pending weights as the previous step set pendingWeights.pending = false.
7. Now the malicious withdrawBounty() has ended, and the original settleAuction() is resuming, but now with malicious weights in pendingWeights (set in step 6). settleAuction() will now call setNewWeights() which will set the basket's weights to be the malicious pending weights.
8. Now settleAuction has finished, and the publisher (within the same transaction) will burn() all his shares of the basket, thereby transferring all the tokens to himself.

POC exploit: Password to both files: "exploit". AttackPublisher.sol , to be put under contracts/contracts/Exploit: https://pastebin.com/efHZjstS ExploitPublisher.test.js , to be put under contracts/test: https://pastebin.com/knBtcWkk

Tools Used

Manual analysis, hardhat.

Recommended Mitigation Steps

In settleAuction(), move basketAsERC20.transfer() and withdrawBounty() to the end of the function, conforming with Checks Effects Interactions pattern.

Handle

kenzo

Vulnerability details

If a user is minting small amount of shares (like 1 - amount depends on baskets weights), the calculated amount of tokens to pull from the user can be less than 1, and therefore no tokens will be pulled. However the shares would still be minted. If the user does this a few times, he could then withdraw the total minted shares and end up with more tokens than he started with - although a miniscule amount.

Impact

User can end up with more tokens than he started with. However, I didn't find a way for the user to get an amount to make this a feasible attack. He gets dust. However he can still get more than he deserves. If for some reason the basket weights grow in a substantial amount, this could give the user more tokens that he didn't pay for.

Proof of Concept

Add the following test to Basket.test.js. The user starts with 5e18 UNI, 1e18 COMP, 1e18 AAVE, and ends with 5e18+4, 1e18+4, 1e18+4.

it("should give to user more than he deserves", async () => {
        await UNI.connect(owner).mint(ethers.BigNumber.from(UNI_WEIGHT).mul(1000000));
        await COMP.connect(owner).mint(ethers.BigNumber.from(COMP_WEIGHT).mul(1000000));
        await AAVE.connect(owner).mint(ethers.BigNumber.from(AAVE_WEIGHT).mul(1000000));
  
        await UNI.connect(owner).approve(basket.address, ethers.BigNumber.from(UNI_WEIGHT).mul(1000000));
        await COMP.connect(owner).approve(basket.address, ethers.BigNumber.from(COMP_WEIGHT).mul(1000000));
        await AAVE.connect(owner).approve(basket.address, ethers.BigNumber.from(AAVE_WEIGHT).mul(1000000));
  
        console.log("User balance before minting:");
        console.log("UNI balance: " + (await UNI.balanceOf(owner.address)).toString());
        console.log("COMP balance: " + (await COMP.balanceOf(owner.address)).toString());
        console.log("AAVE balance: " + (await AAVE.balanceOf(owner.address)).toString());

        
        await basket.connect(owner).mint(ethers.BigNumber.from(1).div(1));
        await basket.connect(owner).mint(ethers.BigNumber.from(1).div(1));
        await basket.connect(owner).mint(ethers.BigNumber.from(1).div(1));
        await basket.connect(owner).mint(ethers.BigNumber.from(1).div(1));
        await basket.connect(owner).mint(ethers.BigNumber.from(1).div(1));

        console.log("\nUser balance after minting 1 share 5 times:");
        console.log("UNI balance: " + (await UNI.balanceOf(owner.address)).toString());
        console.log("COMP balance: " + (await COMP.balanceOf(owner.address)).toString());
        console.log("AAVE balance: " + (await AAVE.balanceOf(owner.address)).toString());

        await basket.connect(owner).burn(await basket.balanceOf(owner.address));
        console.log("\nUser balance after burning all shares:");
        console.log("UNI balance: " + (await UNI.balanceOf(owner.address)).toString());
        console.log("COMP balance: " + (await COMP.balanceOf(owner.address)).toString());
        console.log("AAVE balance: " + (await AAVE.balanceOf(owner.address)).toString());
    });

Tools Used

Manual analysis, hardhat.

Recommended Mitigation Steps

Add a check to pullUnderlying:

require(tokenAmount > 0);

I think it makes sense that if a user is trying to mint an amount so small that no tokens could be pulled from him, the mint request should be denied. Per my tests, for an initial ibRatio, this number (the minimal amount of shares that can be minted) is 2 for weights in magnitude of 1e18, and if the weights are eg. smaller by 100, this number will be 101.

Handle

kenzo

Vulnerability details

A malicious user can listen to the mempool and immediately bond when an auction starts, without aim of settling the auction. As no one can cancel his bond in less than 24h, this will freeze user funds and auction settlement for 24h until his bond is burned and the new index is deleted. The malicious user can then repeat this when a new auction starts.

Impact

Denial of service of the auction mechanism. The malicious user can hold the basket "hostage" and postpone or prevent implementing new index. The only way to mitigate it would be to try to front-run the malicious user, obviously not ideal.

Proof of Concept

publishAllIndex: https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Basket.sol#L170

• The attacker would listen to this function / PublishedNewIndex event and upon catching it, immediately bond the auction.
• The publisher has no way to burn a bond before 24h has passed. But even if he could, it would not really help as the attacker could just bond again (though losing funds in the process). settleAuction: https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L79
• Only the bonder can settle. bondBurn: https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L111
• Can only burn 24h after bond.

Tools Used

Manual analysis, hardhat.

Recommended Mitigation Steps

If we only allow one user to bond, I see no real way to mitigate this attack, because the malicious user could always listen to the mempool and immediately bond when an auction starts and thus lock it. So we can change to a mechanism that allows many people to bond and only one to settle; but at that point, I see no point to the bond mechanism any more. So we might as well remove it and let anybody settle the auction.

With the bond mechanism, a potential settler would have 2 options:

• Bond early: no one else will be able to bond and settle, but the user would need to leave more tokens in the basket (as newRatio starts large and decreases in time)
• Bond late: the settler might make more money as he will need to leave less tokens in the basket, but he risks that somebody else will bond and settle before him. Without a bond mechanism, the potential settler would still have these equivalent 2 options:
• Settle early: take from basket less tokens, but make sure you win the auction
• Settle late: take from basket more tokens, but risk that somebody settles before you So that's really equivalent to the bonding scenario.

I might be missing something but at the moment I see no detriment to removing the bonding mechanism.

Handle

kenzo

Vulnerability details

While handling the fees, the contract calculates the new ibRatio by dividing by totalSupply. This can be 0 leading to a division by 0.

Impact

If everybody burns their shares, in the next mint, totalSupply will be 0, handleFees will revert, and so nobody will be able to use the basket anymore.

Proof of Concept

Vulnerable line: https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Basket.sol#L124 You can add the following test to Basket.test.js and see that it reverts: it("should divide by 0", async () => { await basket.connect(addr1).burn(await basket.balanceOf(addr1.address)); await basket.connect(addr2).burn(await basket.balanceOf(addr2.address));

  await UNI.connect(addr1).approve(basket.address, ethers.BigNumber.from(1));
  await COMP.connect(addr1).approve(basket.address, ethers.BigNumber.from(1));
  await AAVE.connect(addr1).approve(basket.address, ethers.BigNumber.from(1));
  await basket.connect(addr1).mint(ethers.BigNumber.from(1));

});

Tools Used

Manual analysis, hardhat.

Recommended Mitigation Steps

Add a check to handleFees: if totalSupply= 0, you can just return, no need to calculate new ibRatio / fees. You might want to reset ibRatio to BASE at this point.

Handle

kenzo

Vulnerability details

After an auction has started, as time passes and according to the bondTimestamp, newRatio (which starts at 2*ibRatio) gets smaller and smaller and therefore less and less tokens need to remain in the basket. This is not capped, and after a while, newRatio can become smaller than current ibRatio.

Impact

If for some reason nobody has settled an auction and the publisher didn't stop it, a malicious user can wait until newRatio < ibRatio, or even until newRatio ~= 0 (for an initial ibRatio of ~1e18 this happens after less than 3.5 days after auction started), and then bond and settle and steal user funds.

Proof of Concept

These are the vulnerable lines: https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L89:#L99

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

        for (uint256 i = 0; i < pendingWeights.length; i++) {
            uint256 tokensNeeded = basketAsERC20.totalSupply() * pendingWeights[i] * newRatio / BASE / BASE;
            require(IERC20(pendingTokens[i]).balanceOf(address(basket)) >= tokensNeeded);
        }

The function verifies that pendingTokens[i].balanceOf(basket) >= basketAsERC20.totalSupply() * pendingWeights[i] * newRatio / BASE / BASE. This is the formula that will be used later to mint/burn/withdraw user funds. As bondTimestamp increases, newRatio will get smaller, and there is no check on this. After a while we'll arrive at a point where newRatio ~= 0, so tokensNeeded = newRatio*(...) ~= 0, so the attacker could withdraw nearly all the tokens using outputTokens and outputWeights, and leave just scraps in the basket.

Tools Used

Manual analysis, hardhat.

Recommended Mitigation Steps

Your needed condition/math might be different, and you might also choose to burn the bond while you're at it, but I think at the minimum you should add a sanity check in settleAuction:

require (newRatio > basket.ibRatio());