Kuiper contest - 0x0x0x's results

Handle

0x0x0x

Vulnerability details

If someone calls settleAuction and inputs more tokens than the person should, tokens get stuck. To avoid this, it would be better to compute inputTokens/inputWeights and outputTokens/outputWeights automatically, since they can be known beforehand. When a bonder sends a settleAuction request, any change to conditions might lead them to lose their funds(or revert). Furthermore, this is a serious attack vector and for example web hackers can abuse it, without even communicating with a different contract.

Mitigation step

Automate the calculation of inputTokens/inputWeights and outputTokens/outputWeights or add a mechanism to calculate excess amounts for everything and transfer them back to the sender.

Handle

0x0x0x

Vulnerability details

Lets say there is already as many tokens as maxSupply. A user calls burn with amount = 1. handleFees create more than 1 token. Therefore, maxSupply is exceeded. Furthermore, the same can also happen again, when maxSupply is already exceeded.

Handle

0x0x0x

Vulnerability details

When bondPercentDiv is set to 1, it is unlikely that someone can bond by using all tokens (Also because of fees factory would have some tokens). I think this is not intended behaviour.

Mitigation step

Add the condition that this parameter is more than 1 (rather than 0). Another option is to keep it as a lock mechanism, but add a comment about it in factory.

Handle

0x0x0x

Vulnerability details

Impact

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

Occurrences

./Basket.sol:68:        require(_tokens.length > 0);
./Basket.sol:77:            require(_weights[i] > 0);
./Basket.sol:93:        require(amount > 0);
./Basket.sol:110:        require(amount > 0);
./Basket.sol:292:            require(tokenAmount > 0);
./Factory.sol:53:        require(newBondPercentDiv > 0);

Handle

0x0x0x

Vulnerability details

Proof of Concept

Example:

for (uint i = 0; i < arr.length; i++) {

//Operations not effecting the length of the array.

}

Loading length of array costs gas. Therefore, the length should be cached, if the length of the array doesn't change inside the loop. Furthermore, there is no need to assign the initial value 0. This costs extra gas.

Recommended implementation:

uint length = arr.length;

for (uint i; i < length; i++) {

//Operations not effecting the length of the array.

}

By doing so the length is only loaded once rather than loading it as many times as iterations (Therefore, less gas is spent).

Occurences

./Auction.sol:88:        for (uint256 i = 0; i < inputTokens.length; i++) {
./Auction.sol:92:        for (uint256 i = 0; i < outputTokens.length; i++) {
./Auction.sol:105:        for (uint256 i = 0; i < pendingWeights.length; i++) {
./Auction.sol:152:        for (uint256 i = 0; i < bountyIds.length; i++) {
./Basket.sol:75:        for (uint i = 0; i < length; i++) {
./Basket.sol:79:            for (uint256 x = 0; x < tokenList.length; x++) {
./Basket.sol:275:        for (uint256 i = 0; i < weights.length; i++) {
./Basket.sol:282:        for (uint256 i = 0; i < weights.length; i++) {
./Basket.sol:290:        for (uint256 i = 0; i < weights.length; i++) {
./Factory.sol:109:        for (uint256 i = 0; i < bProposal.weights.length; i++) {

Handle

0x0x0x

Vulnerability details

Concept

Negation(!) is a better optimized implementation of applying == false. It requires less gas and also the recommended approach to compute the negation.

In other words: x == false and !x compute the same result, but !x costs less gas.

Scope

./Auction.sol:37:        require(auctionOngoing == false, 'ongoing auction');
./Basket.sol:92:        require(auction.auctionOngoing() == false);
./Basket.sol:109:        require(auction.auctionOngoing() == false);
./Basket.sol:221:            if (auction.auctionOngoing() == false) {
./Basket.sol:259:        require(auction.auctionOngoing() == false);

Handle

0x0x0x

Vulnerability details

basket.sol#mintTo is as follows:

function mintTo(uint256 amount, address to) public nonReentrant override {

require(auction.auctionOngoing() == false);

require(amount > 0);

uint256 startSupply = totalSupply();

require(startSupply + amount <= maxSupply);

handleFees(startSupply);

pullUnderlying(amount, msg.sender);

_mint(to, amount);

require(totalSupply() <= maxSupply);

emit Minted(to, amount);

}

To check, whether maxSupply is exceeded first the following statement is used:

require(startSupply + amount <= maxSupply);

At the end of the function once again, it is checked:

require(totalSupply() <= maxSupply);

Since second requirement already check whether maximum supply is exceeded, the first on is not required and consumes extra gas.

Handle

0x0x0x

Vulnerability details

In handleFees, when timeDiff = 0 the calculations for the fee are not required. Inside last else statement can an if statement added to avoid not needed calculations. This would save gas especially, when the contract is used more than once in a single block.

So this part

} else {
            uint256 timeDiff = (block.timestamp - lastFee);

Can be replaced with the following:

} else {
            uint256 timeDiff = (block.timestamp - lastFee);
						if (timeDiff == 0) {
									return;
						}

Or alternatively all remaining calculations can be inside an if (timeDiff != 0).

Handle

0x0x0x

Vulnerability details

In pushUnderlying and pullUnderlying, to compute the tokenAmount it is required to divide by BASE twice. Square of BASE can be saved in a constant to avoid applying this division twice. Simply a new variable called BASE2 (representing square of BASE) can apply the same logic for less gas.