Back to reassor's contests

Nibbl contest - reassor's results

NFT fractionalization protocol with guaranteed liquidity and price based buyout.

General Information

Platform: Code4rena

Start Date: 21/06/2022

Pot Size: $30,000 USDC

Total HM: 12

Participants: 96

Period: 3 days

Judge: HardlyDifficult

Total Solo HM: 5

Id: 140

League: ETH

Nibbl

Findings Distribution

Researcher Performance

Rank: 3/96

Findings: 3

Award: $2,424.37

🌟 Selected for report: 1

🚀 Solo Findings: 1

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryNibbl

Findings Information

🌟 Selected for report: reassor

Labels

bug
2 (Med Risk)
disagree with severity
sponsor acknowledged

Awards

2339.3444 USDC - $2,339.34

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/NibblVaultFactoryData.sol#L6 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L158-L169 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L37

Vulnerability details

Impact

User can buy out NFT by initiating the process through initiateBuyout, then he has to wait BUYOUT_DURATION which is 5 days and if the buyout will not get rejected he can claim the NFT. During that period bidder cannot cancel the process. The issue is that since NibblVault is used through proxy it is possible to change its implementation through administrative functionality in NibblVaultFactory and the timelock for update'ing implementation is only 2 days.

Attack Scenario:

  1. Bidder initiates buyout through initiateBuyout
  2. Administrator of the protocol updates the vaultImplementation through proposeNewVaultImplementation
  3. Bidder really does not like new implementation but cannot cancel buyout process
  4. Administrator waits 2 days (the UPDATE_TIME) uses updateVaultImplementation and changes the implementation
  5. Bidder loses funds/fait in the protocol

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to either implement functionality for bidder to cancel the bid or increase/decrease the UPDATE_TIME/BUYOUT_DURATION so the invariant BUYOUT_DURATION < UPDATE_TIME holds.

#0 - HardlyDifficult

2022-07-03T00:39:48Z

It seems the bidder could be left in a bad state, and updating the thresholds here may be a nice way to maintain expectations. Since this scenario is based on the admin making an undesirable change, this is a Medium risk report.

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x29A, 0x52, 0xNazgul, 0xNineDec, 0xc0ffEE, 0xf15ers, 0xkatana, BowTiedWardens, Chom, ElKu, Funen, GalloDaSballo, JC, JMukesh, JohnSmith, Lambda, Limbooo, MadWookie, MiloTruck, Nethermind, Noah3o6, Nyamcil, Picodes, PwnedNoMore, Randyyy, RoiEvenHaim, SmartSek, StErMi, Tadashi, TerrierLover, TomJ, Tomio, Treasure-Seeker, UnusualTurtle, Varun_Verma, Wayne, Waze, _Adam, apostle0x01, asutorufos, berndartmueller, c3phas, catchup, cccz, cloudjunky, codexploder, cryptphi, defsec, delfin454000, dipp, ellahi, exd0tpy, fatherOfBlocks, hansfriese, hyh, joestakey, kebabsec, kenta, masterchief, minhquanym, naps62, oyc_109, pashov, peritoflores, reassor, rfa, robee, sach1r0, saian, sashik_eth, shenwilly, simon135, slywaters, sorrynotsorry, sseefried, unforgiven, xiaoming90, ych18, zuhaibmohd, zzzitron

Awards

66.8145 USDC - $66.81

Labels

bug
QA (Quality Assurance)
sponsor acknowledged

External Links

Link to GitHub issue

1. NibblVault sell is missing reentrancy protection

Impact

Function NibbleVault.sell is missing lock modifier that protects against reentrancy attacks.

Proof of Concept

NibblVault.sol:

Tools Used

Manual Review / VSCode

It is recommended to add lock modifier to sell function.

2. Confusing intialize vs initialise

Risk

Low

Impact

Protocol uses proxy pattern for vaults and baskets which requires properly initializing the contracts. The issue is that contract NibblVault implements function initialize and contract Basket.sol implements initialise. Using different naming convention for critical functionality such as initialization makes it prone to errors and leads to vulnerabilities.

Since openzeppelin goes with initializer modifier we recommend using initialize for Basket.sol.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommend to change function Basket.initialise to Basket.initialize.

3. NibblVault incorrect check

Risk

Low

Impact

Contract NibblVault charges admin fee in _chargeFee and _chargeFeeSecondaryCurve:

        if(_adminFeeAmt > 0) {
            safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract
        }

The check is invalid since there might be edge case (when amount is very small) that _adminFeeAmt is positive and _feeAdmin is equal to 0. ETH should be sent when _feeAdmin is positive.

Proof of Concept

NibblVault.sol:

Tools Used

Manual Review / VSCode

It is recommended to change the check to:

        if(_feeAdmin > 0) {
            safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract
        }

or even better after gas optimization to:

        if(_feeAdmin != 0) {
            safeTransferETH(_factory, _feeAdmin); //Transfers admin fee to the factory contract
        }

4. Missing zero address checks

Risk

Low

Impact

Multiple contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

NibblVaultFactory.sol:

NibblVault.sol:

Basket.sol:

Tools Used

Manual Review / VSCode

It is recommended to add zero address checks for listed parameters.

5. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

NibblVaultFactory.sol:

NibblVault.sol:

Twav.sol:

AccessControlMechanism.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing events to listed functions.

6. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

NibblVault.sol:

Tools Used

Manual Review / VSCode

It is recommended to implement two-step process for changing curator address.

7. Missing indexing for events

Risk

Non-Critical

Impact

Events should index addresses which helps off-chain applications in monitoring the protocol.

Proof of Concept

Interfaces/INibblVaultFactory.sol:5:    event Fractionalise(address assetAddress, uint256 assetTokenID, address proxyVault);

Tools Used

Manual Review / VSCode

It is recommended to add indexing to address type parameters.

8. The contracts use unlocked pragma

Risk

Non-Critical

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Proof of Concept

  • (^0.8.0) Utilities/AccessControlMechanism.sol
  • (^0.8.0) Interfaces/IAccessControlMechanism.sol

Tools Used

Manual Review / VSCode

Consider locking compiler version, for example pragma solidity 0.8.10.

9. Misleading constant variable name

Risk

Non-Critical

Impact

Contract NibblVault implement constant variable primaryReserveRatio. The variable is constant so it should follow the naming convention for constant variables PRIMARY_RESERVE_RATIO.

Proof of Concept

NibblVault.sol:

Tools Used

Manual Review / VSCode

It is recommended to change name of primaryReserveRatio to PRIMARY_RESERVE_RATIO.

10. Missing/incomplete natspec comments

Risk

Non-Critical

Impact

Contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

NibblVaultFactory.sol:

NibblVault.sol:

Basket.sol:

Twav.sol:

ProxyVault.sol:

ProxyBasket.sol:

AccessControlMechanism.sol:

EIP721Base.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing natspec comments.

#0 - HardlyDifficult

2022-07-03T22:56:19Z

Merging with https://github.com/code-423n4/2022-06-nibbl-findings/issues/247

#1 - HardlyDifficult

2022-07-04T01:17:16Z

Merging with https://github.com/code-423n4/2022-06-nibbl-findings/issues/250

#2 - HardlyDifficult

2022-07-04T18:56:55Z

Great stuff, very clear report

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0v3rf10w, 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, 8olidity, ACai, BowTiedWardens, Chandr, Chom, ElKu, Fitraldys, Funen, IgnacioB, JC, Lambda, Limbooo, MiloTruck, Noah3o6, Nyamcil, Picodes, Randyyy, SmartSek, StErMi, TerrierLover, TomJ, Tomio, UnusualTurtle, Waze, _Adam, ajtra, c3phas, cRat1st0s, catchup, codexploder, cryptphi, defsec, delfin454000, ellahi, exd0tpy, fatherOfBlocks, hansfriese, joestakey, kebabsec, kenta, m_Rassska, minhquanym, oyc_109, pashov, reassor, rfa, robee, sach1r0, saian, sashik_eth, simon135, slywaters, ych18, ynnad, zuhaibmohd

Awards

18.2244 USDC - $18.22

Labels

bug
duplicate
G (Gas Optimization)

External Links

Link to GitHub issue

1. Variable should be set as constant

Impact

Variable UPDATE_TIME in NibblVaultFactoryData cannot be changed and should be set as constant to save gas.

Proof of Concept

NibblVaultFactoryData.sol:

Tools Used

Manual Review / VSCode

It is recommended to set UPDATE_TIME as constant.

2. Cache array length outside of loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Proof of Concept

Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended to cache the array length outside of for loop.

3. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

NibblVaultFactory.sol:48:        require(msg.value >= MIN_INITIAL_RESERVE_BALANCE, "NibblVaultFactory: Initial reserve balance too low");
NibblVaultFactory.sol:49:        require(IERC721(_assetAddress).ownerOf(_assetTokenID) == msg.sender, "NibblVaultFactory: Invalid sender");
NibblVaultFactory.sol:107:        require(basketUpdateTime != 0 && block.timestamp >= basketUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:131:        require(feeToUpdateTime != 0 && block.timestamp >= feeToUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:141:        require(_newFee <= MAX_ADMIN_FEE, "NibblVaultFactory: Fee value greater than MAX_ADMIN_FEE");
NibblVaultFactory.sol:149:        require(feeAdminUpdateTime != 0 && block.timestamp >= feeAdminUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");
NibblVaultFactory.sol:166:        require(vaultUpdateTime != 0 && block.timestamp >= vaultUpdateTime, "NibblVaultFactory: UPDATE_TIME has not passed");

Tools Used

Manual Review / VSCode

It is recommended to decrease revert messages to maximum 32 bytes.

4. Use custom errors instead of revert strings to save gas

Impact

Usage of custom errors reduces the gas cost.

Proof of Concept

Contracts that should be using custom errors:

  • Basket.sol
  • NibblVaultFactory.sol
  • NibblVault.sol
  • Utilities/AccessControlMechanism.sol

Tools Used

Manual Review / VSCode

It is recommended to add custom errors to listed contracts.

5. Remove obsolete math operations

Impact

Contract NibblVault packs timestamps to uint32 variables. To set blockTimestamp value it first runs math operation modulo on block.timestamp and then casts the result to uint32. Executing modulo operation in this case is obsolete, uint32 will successfully cast the value to 32bits bits.

Proof of Concept

NibblVault.sol:303:            uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
NibblVault.sol:365:            uint32 _blockTimestamp = uint32(block.timestamp % 2**32);
NibblVault.sol:413:        _updateTWAV(_currentValuation, uint32(block.timestamp % 2**32));
NibblVault.sol:445:        uint32 _blockTimestamp = uint32(block.timestamp % 2**32);

Tools Used

Manual Review / VSCode

It is recommended to remove % 2**32 math modulo operation.

6. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

7. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

Proof of Concept

Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended wrap incrementing with unchecked block, for example: unchecked { ++i } or unchecked { --i }

8. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

Basket.sol:43:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:70:        for (uint256 i = 0; i < _tokens.length; i++) {
Basket.sol:93:        for (uint256 i = 0; i < _tokens.length; i++) {
NibblVault.sol:506:        for (uint256 i = 0; i < _assetAddresses.length; i++) {
NibblVault.sol:525:        for (uint256 i = 0; i < _assets.length; i++) {
NibblVault.sol:547:        for (uint256 i = 0; i < _assets.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended to remove explicit initializations with default values.

9. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Proof of Concept

NibblVault.sol:227:        if(_adminFeeAmt > 0) {
NibblVault.sol:243:        if(_adminFeeAmt > 0) {

Tools Used

Manual Review / VSCode

It is recommended to use != 0 instead of > 0.

#0 - mundhrakeshav

2022-06-26T17:09:26Z

#2, #3, #6, #7, #8, #9, #15

AuditHub

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

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter