Nibbl contest - m_Rassska's results

Table of contents

• [G-01] Unnecessary SafeMath imports
• [G-02] Redundant subsequent imports
• [G-03] Defaining custom errors to fetch error messages
• [G-04] Use multiple requires instead of complex equation with extra opcodes
• [G-05] Packing up variables
• [G-06] Using events to track the state of vars on off-chain
• [G-07] Using immutable keyword for storage vars initilized in initilize function()

<br/>

[G-01] Unnecessary SafeMath imports <a name="G-01"></a>

PROBLEM

• Since the versions of all files which import SafeMath are >= 0.8.0, there is no reason for explicit imports. After this PR possible overflow/underflow issues will be reverted by default.

PROOF OF CONCEPT

• Test/Tokens/MaliciousFactory.sol

      10.  import { SafeMath } from  "@openzeppelin/contracts/utils/math/SafeMath.sol";

• Test/TestBancorFormula.sol

      5.   import { SafeMath } from "@openzeppelin/contracts/utils/math/SafeMath.sol";
      14.  using SafeMath for uint256;

• ./NibblVaultFactory.sol

      9.   import { SafeMath } from "@openzeppelin/contracts/utils/math/SafeMath.sol";

TOOLS USED

• Gasol-Optimizer
• Manual Analysis

MITIGATION

• Remove the whole presence of the SafeMath.
• It also includes replacing all SafeMath functions by solidity arithmetic operators.

<br/>

[G-02] Redundant subsequent imports <a name="G-02"></a>

PROBLEM

• If you add new top-level items inside "temp.sol", they automatically appear in all files that import “temp.sol”. In some cases you've specified what you've actually needed, but in another cases - have not.

  

<br/>

PROOF OF CONCEPT

• ./Interfaces/IBasket.sol

      // Redundant import, since ERC721.sol already imports IERC721Receiver.sol
      5.  import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

TOOLS USED

• Gasol-Optimizer
• Manual Analysis

MITIGATION

• Just use {module you need} to specify the context as you did in some places. I'm not 100% sure that I cover all redundant imports according to the current PROBLEM.

<br/>

[G-03] Defaining custom errors to fetch error messages <a name="G-03"></a>

PROBLEM

• It's better to define some custom errors to throw particular signes. So, that later on we'll be able to set those signes with actual messages off-chain making it cheaper to store on-chain.

PROOF OF CONCEPT

• ./Basket.sol

  • The reverting message: "withdraw:not allowed" defined in 8 different require statements within the current scope. Why we can't simply turn it into one single error which will throw some sign like: "W1". Use comments, if you want provide an info about certain sign-codes.

      36.  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
      42.  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
      53.  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
      62.  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
      69.  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
      79.  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
      86.  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
      92.  require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");

• The same is applicable for listed scopes as well.

  • ./NibblVaultFactory.sol
  • ./Test/UpgradedNibblVault.sol
  • ./Test/Tokens/MiliciousFactory.sol

TOOLS USED

• Gasol-Optimizer
• Manual Analysis

MITIGATION

Just simply define custom errors and reverts them whenever it's nedeed.

<br/>

[G-04] Use multiple requires instead of complex equation with extra opcodes <a name="G-04"></a>

PROBLEM

• I don't usually report this type of suggestions, but after seeing complex equations, decided to make it happen. The problem is that for every && or || evm has AND or OR opcodes which cost minimum of 3 gas. Also, by braking down this complex require body into multiple require statements will improve the code readability.

PROOF OF CONCEPT

• ./Bancor/BancorFormula.sol

      188.    require(_supply > 0 && _connectorBalance > 0 && _connectorWeight > 0 && _connectorWeight <= MAX_WEIGHT);
      219.    require(_supply > 0 && _connectorBalance > 0 && _connectorWeight > 0 && _connectorWeight <= MAX_WEIGHT && _sellAmount <= _supply);

• The same is applicable for listed scopes as well.

  • ./NibblVaultFactory.sol
  • ./Test/Tokens/MiliciousFactory.sol

TOOLS USED

• Gasol-Optimizer
• Manual Analysis

MITIGATION

Replacing complex require statement by defining multiple requires.

UPD: I just realized that BancorFormula.sol is not under the scope 😟😟😟.

<br/>

[G-05] Packing up variables <a name="G-05"></a>

PROBLEM

• Storage can operate with 32 bytes slots. And everything here is perfect, expect some small change that I would like to make 😊 in order to use storage effectively.

PROOF OF CONCEPT

• ./Test/UpgradedNibblVault.sol

  •   120. uint private unlocked;

TOOLS USED

• Gasol-Optimizer
• Manual Analysis

MITIGATION

This mutex could be defined as uint8 and placed after lines: 67, 61, because after reserving slot for address(20 bytes) there 12 remaining bytes where we can place our mutex. Congratulations, we saved one slot which is a pretty big deal. 🔥🔥🔥

<br/>

[G-06] Using events to track upgradedNewVariable<a name="G-06"></a>

PROBLEM

• In some cases where there is a need of tracking state of the certain variable off-chain, we can use the power of events rather than allocation the whole slot in a storage.

PROOF OF CONCEPT

• ./Test/UpgradedNibblVault.sol

  •  124. uint256 public upgradedNewVariable;
     ...
     ...
     492. function upgradedFunction() external {
     493.     upgradedNewVariable++;
     494. }

TOOLS USED

• Gasol-Optimizer
• Manual Analysis

MITIGATION

• Delete the storage variable on the line 124.
• Create an event that tracks this specific value and emit it when it's about to change his state. <br/>

[G-07] Using immutable keyword for storage vars initialized in initialize function()<a name="G-07"></a>

PROBLEM

• What is the reason for an allocation such a huge amount of storage memory to store vars which can be declared as immutable.

• Well, the storage variables here are only initialized in initialize function, which will be invoked internally from the NabbiVaultFactory::createVault() by passing each variables to initialize function. Then function will read directly from the calldata and assign all of these vars. This is the only place, where this vars change their states, if it's so, it makes sence to mark them as immutable.

• Making free previously used storage by using delete keyword will not help to retrieve the full gas used for SSTORE.

PROOF OF CONCEPT

• ./Test/UpgradedNibblVault.sol

  •  55.   address payable public factory;   <-- declared as a storage variable
     61.   address public assetAddress;    <-- declared as a storage variable
     64.   uint256 public assetID;   <-- declared as a storage variable
     70.   uint256 public initialTokenPrice;   <-- declared as a storage variable
     75.   uint256 private fictitiousPrimaryReserveBalance;    <-- declared as a storage variable
     ...
     184.  initialTokenPrice=_initialTokenPrice;   <-- here it's initialized
     185.  factory = payable(msg.sender);  <-- here it's initialized
     186.  assetAddress = _assetAddress;   <-- here it's initialized
     187.  assetID = _assetID;   <-- here it's initialized
     192.  fictitiousPrimaryReserveBalance = _primaryReserveBalance;   <-- here it's initialized
     ...
    

  • Well, the storage variables here are only initialized in initialize function, which will be invoked internally from the NabbiVaultFactory::createVault() by passing each variables to initialize function. Then function will read from calldata and assign all of these vars. This is the only place, where this vars change their states, if it's so, it makes sence to mark them as immutable.
  • Freeing previously used storage by using delet keyword will not revert the full gas used for SSTORE, therefore

TOOLS USED

• Gasol-Optimizer
• Manual Analysis

MITIGATION

• I didn't look at every single storage variable which can be turned into immutable var, but my point is all about using immutable where it's should be used. Also, I saw some functions like UpgradedNibblVault::_rejectBayout() which deletes some of them and I was so happy when I saw it, but anyway, it's a sagnificant amount of gas consumption to simply call SSTORE and SLOAD such a huge amount of time.

<br/>

For Judjes

• Hi there! This was my first report on code4rena, sorry for some collapses/clashes. Anyways, huge kudos to all of you for creating such a great project!!!🔥🔥🔥