Kuiper contest - GiveMeTestEther's results

Handle

GiveMeTestEther

Vulnerability details

Impact

Cache "address(basket)" in a local variable to save a few SLOADs. It gets read in three different loops of the settleAuction() function and during the assignment of "basketAsERC20".

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Auction.sol#L74

Tools Used

Manual Analysis

Recommended Mitigation Steps

• new local variable before the first loop: address _basket = address(basket);
• replace the occurrences of address(basket) with the local variable

Handle

GiveMeTestEther

Vulnerability details

Impact

Store the calculation of "basketToken.totalSupply() / factory.bondPercentDiv()" in a local variable. So we can use this local variable in the safeTransfer and the event emit (instead of reading the storage variable again with what we have assigned it in this function). We save a two SLOADs.

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Auction.sol#L59

Tools Used

-Manual Analysis

Recommended Mitigation Steps

• introduce a local variable: uint256 _bondAmount = basketToken.totalSupply() / factory.bondPercentDiv();
• assign to storage: bondAmount = _bondAmount ;
• replace the occurrences of bondAmount in the safeTransfer and event emit.

Handle

GiveMeTestEther

Vulnerability details

Impact

The storage variable bondAmount is read twice in this function. Store it in a local variable and replace the occurrences with the local variable to save a SLOAD.

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Auction.sol#L121

Tools Used

Manual Analysis

Recommended Mitigation Steps

• new local variable: uint256 _bondAmount = bondAmount;
• replace the occurrences of bondAmount with the local variable

Handle

GiveMeTestEther

Vulnerability details

Impact

No need to zero-initialize ownerSplit because default value is zero. Saves gas.

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Factory.sol#L22

Tools Used

Manual Analysis

Recommended Mitigation Steps

• remove L22

Handle

GiveMeTestEther

Vulnerability details

Impact

Reuse function argument "auction_" as argument in calling approveUnderlying on L56 instead of using the storage variable "auction". This saves a cold SLOAD.

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L56

Tools Used

Manual Analysis

Recommended Mitigation Steps

• rewrite L56 as: approveUnderlying(address(auction_));

Handle

GiveMeTestEther

Vulnerability details

Impact

L139: uint256 timeDiff = (block.timestamp - lastFee); timeDiff is always >= 0 because lastFee can only be set to block.timestamp in the same block or in a previous block. Therefore we don't need to check for underflow and can write L139 in an unchecked block

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L139

Tools Used

Manual Analysis

Recommended Mitigation Steps

• write L139 as: unchecked { uint256 timeDiff = (block.timestamp - lastFee); }

Handle

GiveMeTestEther

Vulnerability details

Impact

On L269 & L271 reuse the function argument "newRatio" instead of the storage variable "ibRatio" to save two SLOADs.

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L269 https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L271

Tools Used

Manual Analysis

Recommended Mitigation Steps

• reuse newRatio

Handle

GiveMeTestEther

Vulnerability details

Impact

Assumption is that normal baskets have more than one token in a basket and therefore the loop gets iterated at least twice. The "ibRatio" is storage variable and could be cached in a local variable to save SLOAD in the loops.

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L281 https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L288

Tools Used

Manual Analysis

Recommended Mitigation Steps

• cache ibRation in a local variable

Handle

GiveMeTestEther

Vulnerability details

Impact

The inner loop doesn't need to iterate the whole array length, only needs to compare the current token address (_tokens[i]) until to the last added element to tokenList array in the previous loop iteration.

Proof of Concept

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L75

Tools Used

Manual Anlysis

Recommended Mitigation Steps

function validateWeights(address[] memory _tokens, uint256[] memory _weights) public override pure {
    require(_tokens.length > 0);
    require(_tokens.length == _weights.length);
    uint256 length = _tokens.length;
    address[] memory tokenList = new address[](length);

    // check uniqueness of tokens and not token(0)

   // change: take the first token out from the loop
    require(_tokens[0] != address(0));
    require(_weights[0] > 0);
    tokenList[0] = _tokens[0];

    // change: start at i = 1
    for (uint i = 1; i < length; i++) {
        require(_tokens[i] != address(0));
        require(_weights[i] > 0);

        // change: only compare the previous added tokenList elements and not the others that have initial 0-value => loop condition changed to "x < i"
        for (uint256 x = 0; x < i; x++) {
            require(_tokens[i] != tokenList[x]);
        }

        tokenList[i] = _tokens[i];
    }
}