Nibbl contest - simon135's results

1. unused import statments

nibblVaultFactory.sol is already importing everything from NibblVault.sol which has Ierc20.sol ,Ierc1155.sol,IERC721.sol already imported from it . Remove Ierc20.sol and Ierc1155.sol ,IERC721.sol from nibblVaultFactory.sol. instances include: nibblVaultFactory.sol :4,5,6

2. there are no emits of events after updateing importent variables and the old variable

nibblVaultFactory.sol :102,109,125,143,160,168

no check on address zero

nibblVaultFactory.sol :100,123,140,158

not use erc20upgradeable it can brake

if it gets upgraded and cause weird logic nibblvault:1

typos

instead of seconday use:secondary https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVault.sol#L263 instead of continous use: continuos https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVault.sol#L282 instead of : recieve use: receive https://github.com/NibblNFT/nibbl-smartcontracts/blob/49bf364d9e81a554cfdf47ae5cfc3daf52a54ad6/contracts/NibblVault.sol#L361

Attacker can possibly mint tokens he dosnt pay for

Discalimer i dint know if this was an exploit so its in qa _minAmtOut dosent have to fail if you supply zero becaue anything in purchaseReturn is greater or equal to it. attacker can take advangate to this possibly steal tokens or brake logic and maybe even mint tokens for himself if attacker dosnt pay the fees and if purchaseReturn is more than zero lets say 1 wei then no check for such a small amount that he is going to work and possible get through alot of bypass of your contract.

1. Attacker calls buy() function and _minAmtOut=0 and _to=AttackerADDRERESS
2. since there is no check for anything that small of amount attacker dosnt pay fees.
3. mints more then 1 wei of tokens then supply and contract execpted and since there is no _minAmtOut check
4. Attacker mints small amont of tokens at time same thing with the sell function

mitigation

require (_minAmtOut>0);

1. unused import statments can also save gas

nibblVaultFactory.sol is already importing everything from NibblVault.sol which has Ierc20.sol ,Ierc1155.sol,IERC721.sol already imported from it . Remove Ierc20.sol and Ierc1155.sol ,IERC721.sol from nibblVaultFactory.sol. instances include: nibblVaultFactory.sol :4,5,6

2. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Source Custom Errors in Solidity: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). instances include: nibblVaultFactory.sol: 48,49,107,131,141,149,166 nibblVault.sol:139,129,146,147,154,184,185,

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc. 1 byte for character

nibblVaultFactory.sol: 48,49,107,131,141,149,166,325,351,387,399,400,404,444,475,486,496,505,516,524,536,546,561,570,

make onlyadmin function payable to save gas

Make functions with governance modifier payable saves gas because payable function dont have to check msg.value ==0 and since it doesn't affect the function because governance is only people to call the function it saves gas.

nibblVaultFactory.sol: 99,123,140,158,173,179 Basket.sol: almost the whole contract is approve by owner AccessControlMechanism.sol:32,40,47

use mutiple require statements instead of && to save gas

nibblVaultFactory.sol:107,131,149,166 Basket.sol:36,42,53,62,69,79,86,93

make variables uninitlized to save gas

because variables uninitlized in evm is still 0 and saves 3 gas nibblVault.sol:506,525,547 Basket.sol::43,70,93

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled. i++ increments i and returns the initial value of i. Which means: uint i = 1; i++; // == 1 but i == 2 But ++i returns the actual incremented value: uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 instances include: nibblVault.sol:506,525,547,561 Basket.sol::43,70,93

make array.length cached to a variable

nibblVault.sol:506,525,547 Basket.sol::43,70,93

Unchecking arithmetics operations that can’t underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: Checked or Unchecked Arithmetic. I suggest wrapping with an unchecked block: Same thing with second unchecked because total can't overflow amount cant overflow nibblVault.sol:506,525,547 Basket.sol::43,70,93

you can make unlucked uninlized in the beigning because your initlizing it in the constructor to save gas

nibblVault.sol:126

do keccak256 function of change not just assign the variable on chain to save gas

nibblVault.sol:51 AccessControlMechanism.sol:12,13,14

you can constant variables immutable to save gas in certin cases like with getting keccak hash assign on constant

nibblVault.sol:51 AccessControlMechanism.sol:12,13,14

waste of gas in struct becaue uint256 takes of more gas then a uint32 because of how the evm works, the evm has to bit wise converion on uint 256 to turn into a uint32

Twav.sol:6

Using bools for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled. AccessControlMechanism.sol:16

dont use abi.encode instead use abi.encodepacked to save gas

abi.encodepacked dosnt pad zeros EIP721Base.sol#L16