Kuiper contest - bw's results

Automated portfolio protocol.

General Information

Platform: Code4rena

Start Date: 16/09/2021

Pot Size: $50,000 USDC

Total HM: 26

Participants: 30

Period: 7 days

Judge: GalloDaSballo

Total Solo HM: 17

Id: 36

League: ETH

Kuiper

Findings Distribution

Researcher Performance

Rank: 21/30

Findings: 3

Award: $359.37

🌟 Selected for report: 2

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Kuiper

Findings Information

🌟 Selected for report: 0xRajeev

Also found by: bw, pauliax

Labels

bug

duplicate

2 (Med Risk)

sponsor confirmed

Awards

270.5216 USDC - $270.52

External Links

Link to GitHub issue

Handle

Vulnerability details

Impact

Bounty is read into memory and then modified, which does not result in the modification being written to storage.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Auction.sol#L143

Modifications (_bounties public)

--- a/contracts/contracts/Auction.sol
+++ b/contracts/contracts/Auction.sol
@@ -24,7 +24,7 @@ contract Auction is IAuction {
     IFactory public override factory;
     address public override auctionBonder;
 
-    Bounty[] private _bounties;
+    Bounty[] public _bounties;
 
     modifier onlyBasket() {
         require(msg.sender == address(basket), 'not basket');
diff --git a/contracts/test/Auction.test.js b/contracts/test/Auction.test.js
index 5377bbc..9cbb126 100644
--- a/contracts/test/Auction.test.js
+++ b/contracts/test/Auction.test.js
@@ -142,7 +142,8 @@ describe("Auction", function () {
         await UNI.approve(auction.address, `100000000000000000000`);
         await COMP.approve(auction.address, `100000000000000000000`);
         await AAVE.approve(auction.address, `100000000000000000000`);
-
-        await expect(auction.settleAuction([], [COMP.address], ["2999400000000000000"], [UNI.address, AAVE.address], ["200720000000000000", "200120000000000000"])).to.be.ok;
+        await auction.addBounty(UNI.address, `100000000000000000000`);
+        await expect(auction.settleAuction([0], [COMP.address], ["2999400000000000000"], [UNI.address, AAVE.address], ["200720000000000000", "200120000000000000"])).to.be.ok;
+        console.log(`bounty.active = ${await auction._bounties(0)}`);
       });
 });

Output

bounty = 0x0E801D84Fa97b50751Dbf25036d067dCf18858bF,100000000000000000000,true

Should have returned false.

Tools Used

N/A

Recommended Mitigation Steps

The memory keyword should be replaced with storage in order for bounty.active = false; to be persisted.

diff --git a/contracts/contracts/Auction.sol b/contracts/contracts/Auction.sol
index f07df8b..0ec5efe 100644
--- a/contracts/contracts/Auction.sol
+++ b/contracts/contracts/Auction.sol
@@ -140,7 +140,7 @@ contract Auction is IAuction {
     function withdrawBounty(uint256[] memory bountyIds) internal {
         // withdraw bounties
         for (uint256 i = 0; i < bountyIds.length; i++) {
-            Bounty memory bounty = _bounties[bountyIds[i]];
+            Bounty storage bounty = _bounties[bountyIds[i]];
             require(bounty.active);
 
             IERC20(bounty.token).transfer(msg.sender, bounty.amount);

#0 - frank-beard
2021-10-19T17:49:13Z

https://github.com/code-423n4/2021-09-defiprotocol-findings/issues/168

#1 - GalloDaSballo
2021-12-20T01:03:00Z

Finding is valid, in lack of any POC nor explanation for possible implications with the finding, I believe low severity to be correct

#2 - GalloDaSballo
2021-12-21T22:31:54Z

After re-reviewing, I've increased severity of a similar finding, and since this is a duplicate of it, am increasing the severity of this to medium as well.

Duplicate of #168

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Auditors

Browse

Contests

Browse

Get in touch

Contact Twitter

Findings Information

🌟 Selected for report: bw

Also found by: 0xRajeev, hrkrshnn

Labels

bug

duplicate

G (Gas Optimization)

sponsor confirmed

Awards

15.5766 USDC - $15.58

External Links

Link to GitHub issue

Handle

Vulnerability details

Impact

The Factory.sol contract made use of a number of public variables that were set only in the constructor and would remain constant. These variables were consuming storage slots, which unnecessarily increased the deployment and runtime gas costs of the contract.

For more information regarding the immutable keyword: https://blog.soliditylang.org/2020/05/13/immutable-keyword/

Proof of Concept

Code Diff

diff --git a/contracts/contracts/Factory.sol b/contracts/contracts/Factory.sol
index 271945d..3bbdd4f 100644
--- a/contracts/contracts/Factory.sol
+++ b/contracts/contracts/Factory.sol
@@ -23,8 +23,8 @@ contract Factory is IFactory, Ownable {
 
     Proposal[] private _proposals;
 
-    IAuction public override auctionImpl;
-    IBasket public override basketImpl;
+    IAuction public immutable override auctionImpl;
+    IBasket public immutable override basketImpl;
 
     uint256 public override minLicenseFee = 1e15; // 1e15 0.1%
     uint256 public override auctionDecrement = 10000;

Gas Improvement

diff --git a/base.gas b/factory-immutable.gas
index 9d48ade..1447433 100644
--- a/base.gas
+++ b/factory-immutable.gas
@@ -23,9 +23,9 @@
 ·····················|························|·············|·············|···········|···············|··············
 |  ERC20Upgradeable  ·  approve               ·          -  ·          -  ·    48900  ·            3  ·          -  │
 ·····················|························|·············|·············|···········|···············|··············
-|  Factory           ·  createBasket          ·     880031  ·     908831  ·   882911  ·           10  ·          -  │
+|  Factory           ·  createBasket          ·     875780  ·     904580  ·   878660  ·           10  ·          -  │
 ·····················|························|·············|·············|···········|···············|··············
-|  Factory           ·  proposeBasketLicense  ·     335488  ·     335512  ·   335505  ·           12  ·          -  │
+|  Factory           ·  proposeBasketLicense  ·     333388  ·     333412  ·   333405  ·           12  ·          -  │
 ·····················|························|·············|·············|···········|···············|··············
 |  Factory           ·  setOwnerSplit         ·          -  ·          -  ·    46173  ·            1  ·          -  │
 ·····················|························|·············|·············|···········|···············|··············
@@ -39,7 +39,7 @@
 ··············································|·············|·············|···········|···············|··············
 |  Basket                                     ·          -  ·          -  ·  2390793  ·          8 %  ·          -  │
 ··············································|·············|·············|···········|···············|··············
-|  Factory                                    ·          -  ·          -  ·  1706801  ·        5.7 %  ·          -  │
+|  Factory                                    ·          -  ·          -  ·  1684215  ·        5.6 %  ·          -  │
 ··············································|·············|·············|···········|···············|··············
 |  TestToken                                  ·     653145  ·     653193  ·   653163  ·        2.2 %  ·          -  │
 ·---------------------------------------------|-------------|-------------|-----------|---------------|-------------·

By removing the public keyword from all variables that are not required (which are only used in the unit tests), the deployment costs can be further reduced.

Tools Used

https://www.npmjs.com/package/hardhat-gas-reporter

Recommended Mitigation Steps

Add the immutable key word to all variables that are only set during the constructor.

#0 - GalloDaSballo
2021-11-29T13:56:33Z

Finding is valid, great poc with details

Findings Information

🌟 Selected for report: gpersoon

Also found by: WatchPug, bw

Labels

bug

duplicate

G (Gas Optimization)

sponsor confirmed

Awards

15.5766 USDC - $15.58

External Links

Link to GitHub issue

Handle

Vulnerability details

Impact

The handeFees() function in Basket.sol wastes gas if the function is triggered more than once per block. This is due to the timeDiff being zero on subsequently calls and all resulting calculations subsequently also resulting in zero.

Proof of Concept

Code diff

index 5fef21b..24e80e6 100644
--- a/contracts/contracts/Basket.sol
+++ b/contracts/contracts/Basket.sol
@@ -114,6 +114,7 @@ contract Basket is IBasket, ERC20Upgradeable {
             uint256 startSupply = totalSupply();
 
             uint256 timeDiff = (block.timestamp - lastFee);
+            if (timeDiff == 0) return;
             uint256 feePct = timeDiff * licenseFee / ONE_YEAR;
             uint256 fee = startSupply * feePct / (BASE - feePct);

Tools Used

https://www.npmjs.com/package/hardhat-gas-reporter

Recommended Mitigation Steps

Return early if handleFees has already been called in the current block, prevent unnecessary calls to _mint and rewriting storage slots with the same value.

#0 - GalloDaSballo
2021-11-28T17:24:51Z

Duplicate of #53

Findings Information

🌟 Selected for report: bw

Labels

bug

G (Gas Optimization)

sponsor confirmed

Awards

57.691 USDC - $57.69

External Links

Link to GitHub issue

Handle

Vulnerability details

Impact

A require statement in Factory.sol could be performed prior to an expensive cross contract call, reducing the amount of gas wasted if the validation fails.

Proof of Concept

https://github.com/code-423n4/2021-09-defiProtocol/blob/main/contracts/contracts/Factory.sol#L74

Tools Used

N/A

Recommended Mitigation Steps

Move the require statement before basketImpl.validateWeights(tokens, weights);

#0 - GalloDaSballo
2021-11-29T13:54:47Z

Agree with the finding, putting the require first saves gas in the worst case and has no impact in the best case

Kuiper contest - bw's results

Automated portfolio protocol.

General Information

Findings Distribution

Researcher Performance

External Links

[M-22] Changes made to memory and not storage - $270.52

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Modifications (_bounties public)

Output

Tools Used

Recommended Mitigation Steps

#0 - frank-beard2021-10-19T17:49:13Z

#1 - GalloDaSballo2021-12-20T01:03:00Z

#2 - GalloDaSballo2021-12-21T22:31:54Z

Gas Report - $88.85

Gas Report - $88.85

🌟 [G-19] Runtime constants not defined as immutable - $15.58

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Code Diff

Gas Improvement

Tools Used

Recommended Mitigation Steps

#0 - GalloDaSballo2021-11-29T13:56:33Z

[G-23] `handleFees()` wasting gas if called multiple times in the same block - $15.58

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Code diff

Tools Used

Recommended Mitigation Steps

#0 - GalloDaSballo2021-11-28T17:24:51Z

🚀 [G-33] Require statement can be moved to start of function - $57.69

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - GalloDaSballo2021-11-29T13:54:47Z

#0 - frank-beard
2021-10-19T17:49:13Z

#1 - GalloDaSballo
2021-12-20T01:03:00Z

#2 - GalloDaSballo
2021-12-21T22:31:54Z

#0 - GalloDaSballo
2021-11-29T13:56:33Z

#0 - GalloDaSballo
2021-11-28T17:24:51Z

#0 - GalloDaSballo
2021-11-29T13:54:47Z