Kuiper contest - itsmeSTYJ's results

Handle

itsmeSTYJ

Vulnerability details

Impact

All funds in the basket can be stolen.

Proof of Concept

1. Malicious user creates a smart contract that can interact w/ the auction (can call bondForRebalance and settleAuction).
2. Auction is started by the publisher
3. Malicious user uses smart contract to call bondForRebalance() to give him time to lock his position as a bonder.
4. Before calling settleAuction, he can deterministically calculate a, b, newRatio, tokensNeeded.
5. With these values in hand, he can create add multiple new malicious bounties that matches the pendingTokens and the tokens needed to pass the check on lines 96-98.
6. He will call settleAuction with the follow params settleAuction([malicious bounty IDs], [], [], [all tokens in the basket], [entire token balances excluding the amount needed to pass lines 96-98])
7. lines 96-98 are executed and passes successfully
8. When withdrawBounty is called, it will remove the remaining funds in the contract that was used to pass lines 96-98.

Note that lines 4-6 are all executed atomically within 1 function call.

Recommended Mitigation Steps

Check balances after the bounties have been withdrawn instead of the other way round.

Handle

itsmeSTYJ

Vulnerability details

Impact

Unsafe ERC20 tokens that are compiled before solidity version 0.4.22 will not be able to be transferred out of the contract as the transfer function does not return a value on transfer i.e. they do not 100% comply w/ the current ERC20 token standard.

This means if these unsafe tokens are used to fund the bounty, the tokens are lost forever. You can read more about this here.

Recommended Mitigation Steps

function withdrawBounty(uint256[] memory bountyIds) internal {
    // withdraw bounties
    for (uint256 i = 0; i < bountyIds.length; i++) {
        Bounty memory bounty = _bounties[bountyIds[i]];
        require(bounty.active);

        IERC20(bounty.token).safeTransfer(msg.sender, bounty.amount); // replaced transfer with safeTransfer
        bounty.active = false;

        emit BountyClaimed(msg.sender, bounty.token, bounty.amount, bountyIds[i]);
    }
}

Handle

itsmeSTYJ

Vulnerability details

Impact

USDT is the most popular example of this where there exists a flag to turn on a "fee per transaction". It is currently turned off but you do not know when it'll turn on. There might be more tokens like these in the future where a portion of the amount will be taken as fees whenever the tokens are transferred.

You want to make sure your protocol can handle edgecases like these where the amount that is requested to be transferred is less than the actual amount transferred.

Recommended Mitigation Steps

I don't think there's a correct way to mitigate against this but as a start, you should check the tokenBalance before and after the transfer to get the actual amount transferred. Once you have the actual amount of tokens transferred you can either:

1. revert the entire transaction if it's less than the amount specified
2. keep track of the lowest amount of tokens transferred, refund the excess tokens
3. keep track of the lowest amount of tokens, ignore excess

I would go for option 2 because it allows your protocol to use these tokens in the basket as opposed to rejecting them and it's easier to explain why the excess are refunded than to explain why the excess are kept.

Note that this fix should be applied to all lines of code where safeTransferFrom is used.

Handle

itsmeSTYJ

Vulnerability details

Impact

More fees are actually charged than intended

Mitigation Steps

Basket.sol line 118

Assume that license fee is 10% i.e. 1e17 and time diff = half a year.

When you calculate feePct, you expect to get 5e16 since that's 5% and the actual amount of fee to be charged should be totalSupply * feePct (5) / BASE (100) but on line 118, we are actually dividing by BASE - feePct i.e. 95.

5 / 95 = 0.052 instead of the intended 0.05.

Solution is to replace BASE - feePct in the denominator with BASE.

Handle

itsmeSTYJ

Vulnerability details

Impact

Normally I would classify this as low but because of how widespread USDT is, I believe it is important to handle this case simply for USDT. USDT doesn't allow you to approve allowance unless you first set it to 0.

If your basket contains USDT initially and the publisher tries to publishNewIndex with USDT (regardless of weighting), any bonder that tries to settle the auction will always fail because basket.setNewWeights will always fail when it comes to approving max allowance for USDT.

The publisher might not be malicious but the publisher not knowing about this is not an excuse for users losing their bond.

Recommended Mitigation Steps

function approveUnderlying(address spender) private {
    for (uint256 i = 0; i < weights.length; i++) {
				if(IERC20(tokens[i]).allowance(msg.sender, spender) > 0) {
						IERC20(tokens[i]).approve(spender, 0); // checks if allowance is non 0, approve to 0.
				}
        IERC20(tokens[i]).approve(spender, type(uint256).max);
    }
}

Handle

itsmeSTYJ

Vulnerability details

Impact

Worst case - no functions that contains handleFees() can pass because line 118 will always underflow and revert. You only need feePct to be bigger than BASE for the handleFees() function to fail which will result in a lot of gas wasted and potentially bond burnt.

I did not classify this as high risk because a simple fix would simply to reduce the licenseFee via changeLicenseFee.

Recommended Mitigation Steps

Add these require statement to the following functions:

• Basket.changeLicenseFee()
  • require(newLicenseFee <= BASE, "changeLicenseFee: license fee cannot be greater than 100%");
• Factory.proposeBasketLicense()
  • require(licenseFee <= BASE, "proposeBasketLicense: license fee cannot be greater than 100%");