GiveMeTestEther - Nested Finance contest

Findings Information

🌟 Selected for report: GiveMeTestEther

Also found by: pauliax

Labels

bug

1 (Low Risk)

sponsor confirmed

Awards

389.9738 USDC - $389.97

External Links

Link to GitHub issue

Handle

GiveMeTestEther

Vulnerability details

Impact

The storage variable totalWeights can be set 0 by onlyOwner and therefore we would have a division by zero in the function "_computeShareCount" https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L268

Proof of Concept

-Assumption we have one shareholder with weight S1 > 0 and royaltiesWeight > 0. -With the function updateShareholder the onlyOwner sets the S1 of our shareholder to 0. - updateShareholder: https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L166 - require(_totalWeights > 0, "FeeSplitter: TOTAL_WEIGHTS_ZERO") condition is met because _totalWeights is the sum of all shareholder weights + royaltiesWeight, and royaltiesWeight is > 0

With the function setRoyaltiesWeight the onlyOwner sets the royaltiesWeight to 0.
- setRoyaltiesWeight: https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L94
=> setRoyaltiesWeight is 0 and totalWeights is 0

Tools Used

Manual Analysis

Recommended Mitigation Steps

At the end of the function setRoyaltiesWeight check for 0 weight with a require: require(totalWeights > 0, "FeeSplitter: TOTAL_WEIGHTS_ZERO"); https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L94

Findings Information

🌟 Selected for report: 0xngndev

Also found by: GiveMeTestEther, WatchPug, gzeon, pauliax, ye0lde

Labels

bug

duplicate

G (Gas Optimization)

Awards

10.4335 USDC - $10.43

External Links

Link to GitHub issue

Handle

GiveMeTestEther

Vulnerability details

Impact

Reason strings for reverts that don't fit into 32 bytes will increase contract creation gas and and also the gas during runtime if a require condition is not met (at least one additional mstore + othe overhead).

Proof of Concept

2021-11-nested\contracts\FeeSplitter.sol: row 74: require(_msgSender() == weth, "FeeSplitter: ETH_SENDER_NOT_WETH"); 2021-11-nested\contracts\FeeSplitter.sol: row 157: require(_royaltiesTarget != address(0), "FeeSplitter: INVALID_ROYALTIES_TARGET_ADDRESS"); 2021-11-nested\contracts\FeeSplitter.sol: row 167: require(_accountIndex + 1 <= shareholders.length, "FeeSplitter: INVALID_ACCOUNT_INDEX"); 2021-11-nested\contracts\NestedAsset.sol: row 34: require(supportedFactories[_msgSender()], "NestedAsset: FORBIDDEN_NOT_FACTORY"); 2021-11-nested\contracts\NestedAsset.sol: row 40: require(_address == ownerOf(_tokenId), "NestedAsset: FORBIDDEN_NOT_OWNER"); 2021-11-nested\contracts\NestedAsset.sol: row 115: require(bytes(tokenURI(_tokenId)).length == 0, "NestedAsset: TOKEN_URI_IMMUTABLE"); 2021-11-nested\contracts\NestedAsset.sol: row 142: require(supportedFactories[_factory] == true, "NestedAsset: ALREADY_NOT_SUPPORTED"); 2021-11-nested\contracts\NestedBuybacker.sol: row 52: require(_burnPercentage <= 1000, "NestedBuybacker::constructor: Burn part to high"); 2021-11-nested\contracts\NestedBuybacker.sol: row 82: require(_burnPercentage <= 1000, "NestedBuybacker::setBurnPart: Burn part to high"); 2021-11-nested\contracts\NestedFactory.sol: row 56: require(nestedAsset.ownerOf(_nftId) == _msgSender(), "NestedFactory: Not the token owner"); 2021-11-nested\contracts\NestedFactory.sol: row 64: require(block.timestamp > nestedRecords.getLockTimestamp(_nftId), "NestedFactory: The NFT is currently locked"); 2021-11-nested\contracts\NestedFactory.sol: row 90: require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator"); 2021-11-nested\contracts\NestedFactory.sol: row 97: require(address(reserve) == address(0), "NestedFactory::setReserve: Reserve is immutable"); 2021-11-nested\contracts\NestedFactory.sol: row 104: require(address(_feeSplitter) != address(0), "NestedFactory::setFeeSplitter: Invalid feeSplitter address"); 2021-11-nested\contracts\NestedFactory.sol: row 117: require(_orders.length > 0, "NestedFactory::create: Missing orders"); 2021-11-nested\contracts\NestedFactory.sol: row 134: require(_orders.length > 0, "NestedFactory::addTokens: Missing orders"); 2021-11-nested\contracts\NestedFactory.sol: row 150: require(_orders.length > 0, "NestedFactory::swapTokenForTokens: Missing orders"); 2021-11-nested\contracts\NestedFactory.sol: row 169: require(_orders.length > 0, "NestedFactory::sellTokensToNft: Missing orders"); 2021-11-nested\contracts\NestedFactory.sol: row 170: require(_sellTokensAmount.length == _orders.length, "NestedFactory::sellTokensToNft: Input lengths must match"); 2021-11-nested\contracts\NestedFactory.sol: row 189: require(_orders.length > 0, "NestedFactory::sellTokensToWallet: Missing orders"); 2021-11-nested\contracts\NestedFactory.sol: row 220: require(_orders.length > 0, "NestedFactory::destroy: Missing orders"); 2021-11-nested\contracts\NestedFactory.sol: row 221: require(tokens.length == _orders.length, "NestedFactory::destroy: Missing sell args"); 2021-11-nested\contracts\NestedFactory.sol: row 262: require(assetTokensLength > _tokenIndex, "NestedFactory::withdraw: Invalid token index"); 2021-11-nested\contracts\NestedFactory.sol: row 264: require(assetTokensLength > 1, "NestedFactory::withdraw: Can't withdraw the last asset"); 2021-11-nested\contracts\NestedFactory.sol: row 391: require(success, "NestedFactory::_submitOrder: Operator call failed"); 2021-11-nested\contracts\NestedFactory.sol: row 470: require(holding.amount >= _inputTokenAmount, "NestedFactory:_transferInputTokens: Insufficient amount"); 2021-11-nested\contracts\NestedFactory.sol: row 475: require(msg.value == _inputTokenAmount, "NestedFactory::_transferInputTokens: Insufficient amount in"); 2021-11-nested\contracts\NestedRecords.sol: row 171: require(_maxHoldingsCount > 0, "NestedRecords: INVALID_MAX_HOLDINGS"); 2021-11-nested\contracts\OperatorResolver.sol: row 43: require(names.length == destinations.length, "OperatorResolver::importOperators: Input lengths must match"); 2021-11-nested\contracts\libraries\OperatorHelpers.sol: row 51: require(tokens[0] == _outputToken, "OperatorHelpers::getDecodeDataAndRequire: Wrong output token"); 2021-11-nested\contracts\libraries\OperatorHelpers.sol: row 52: require(tokens[1] == _inputToken, "OperatorHelpers::getDecodeDataAndRequire: Wrong input token"); 2021-11-nested\contracts\operators\Flat\FlatOperator.sol: row 18: require(amount > 0, "FlatOperator::commitAndRevert: Amount must be greater than zero"); 2021-11-nested\contracts\operators\ZeroEx\ZeroExOperator.sol: row 38: require(success, "ZeroExOperator::commitAndRevert: 0x swap failed");

Tools Used

Python script

Recommended Mitigation Steps

Shorten the reason strings to max 32 bytes

#0 - maximebrugel
2021-11-18T14:05:25Z

Duplicated : #14

Findings Information

🌟 Selected for report: GiveMeTestEther

Also found by: pants

Labels

bug

G (Gas Optimization)

sponsor acknowledged

Awards

47.7068 USDC - $47.71

External Links

Link to GitHub issue

Handle

GiveMeTestEther

Vulnerability details

Impact

The loop index increments can be written as unchecked { ++i } instead of simply i++ to save gas. Two reasons:

unchecked because the loop variable i is of type uint256, therefore we would run out of gas before triggering an overflow
++i instead of i++ because it is slightly more efficient

Proof of Concept

see discussion here https://github.com/ethereum/solidity/issues/10695

Tools Used

Visual Code Studio

Recommended Mitigation Steps

replace i++ with unchecked { ++i }

#0 - adrien-supizet
2021-11-18T12:01:44Z

As already discussed here in another audit https://github.com/code-423n4/2021-10-pooltogether-findings/issues/6#issuecomment-942583925

We are not going to fix this as it reduces clarity

Findings Information

🌟 Selected for report: GiveMeTestEther

Also found by: WatchPug

Labels

bug

G (Gas Optimization)

sponsor confirmed

Awards

47.7068 USDC - $47.71

External Links

Link to GitHub issue

Handle

GiveMeTestEther

Vulnerability details

Impact

Calculation of the weights in the function "setRoyaltiesWeight" of FeeSplitter.sol (row 95) can be done more gas efficient.

function setRoyaltiesWeight(uint256 _weight) public onlyOwner {
    totalWeights -= royaltiesWeight;
    royaltiesWeight = _weight;
    totalWeights += _weight;
}

can be rewritten as

function setRoyaltiesWeight(uint256 _weight) public onlyOwner {
    totalWeights = totalWeights - royaltiesWeight + _weight;
    royaltiesWeight = _weight;
}

=> write only once to the storage of totalWeights.

Proof of Concept

Tools Used

Visual Studio Code

Recommended Mitigation Steps

see above

#0 - maximebrugel
2021-11-17T16:36:03Z

Before:

After:

Findings Information

🌟 Selected for report: GiveMeTestEther

Also found by: defsec

Labels

bug

G (Gas Optimization)

sponsor confirmed

Awards

47.7068 USDC - $47.71

External Links

Link to GitHub issue

Handle

GiveMeTestEther

Vulnerability details

Impact

Functions in the same contract with the same access modifier (e.g. onlyOwner or onlyFactory) have have a mix of public and external visibility. Set their visibility to external to save gas.

Affected contracts (see ):

NestedRecords
NestedFactory
FeeSplitter
NestedAsset

Tools Used

Visial Studio Code + Solidity Visual Developer (Plugin)

Recommended Mitigation Steps

Set the visibility to external to save gas.

Extract from Solidity Visual Developer (Plugin) of the Contracts and visibility:

Contract	Type	Bases
└	Function Name	Visibility	Mutability	Modifiers

NestedRecords	Implementation	Ownable
└	<Constructor>	Public ❗️	🛑	NO❗️
└	createRecord	Public ❗️	🛑	onlyFactory
└	updateHoldingAmount	Public ❗️	🛑	onlyFactory
└	getAssetTokens	Public ❗️		NO❗️
└	freeHolding	Public ❗️	🛑	onlyFactory
└	store	External ❗️	🛑	onlyFactory
└	getAssetHolding	External ❗️		NO❗️
└	setFactory	External ❗️	🛑	onlyOwner
└	updateLockTimestamp	External ❗️	🛑	onlyFactory
└	setMaxHoldingsCount	External ❗️	🛑	onlyOwner
└	getAssetReserve	External ❗️		NO❗️
└	getAssetTokensLength	External ❗️		NO❗️
└	getLockTimestamp	External ❗️		NO❗️
└	setReserve	External ❗️	🛑	onlyFactory
└	removeNFT	External ❗️	🛑	onlyFactory
└	deleteAsset	Public ❗️	🛑	onlyFactory
└	freeToken	Private 🔐	🛑

NestedFactory	Implementation	INestedFactory, ReentrancyGuard, Ownable, MixinOperatorResolver, Multicall
└	<Constructor>	Public ❗️	🛑	MixinOperatorResolver
└	<Receive Ether>	External ❗️	💵	NO❗️
└	resolverAddressesRequired	Public ❗️		NO❗️
└	addOperator	External ❗️	🛑	onlyOwner
└	removeOperator	External ❗️	🛑	onlyOwner
└	setReserve	External ❗️	🛑	onlyOwner
└	setFeeSplitter	External ❗️	🛑	onlyOwner
└	create	External ❗️	💵	nonReentrant
└	addTokens	External ❗️	💵	nonReentrant onlyTokenOwner
└	swapTokenForTokens	External ❗️	🛑	nonReentrant onlyTokenOwner isUnlocked
└	sellTokensToNft	External ❗️	🛑	nonReentrant onlyTokenOwner isUnlocked
└	sellTokensToWallet	External ❗️	🛑	nonReentrant onlyTokenOwner isUnlocked
└	destroy	External ❗️	🛑	nonReentrant onlyTokenOwner isUnlocked
└	withdraw	External ❗️	🛑	nonReentrant onlyTokenOwner isUnlocked
└	increaseLockTimestamp	External ❗️	🛑	onlyTokenOwner
└	unlockTokens	External ❗️	🛑	onlyOwner
└	_submitInOrders	Private 🔐	🛑
└	_submitOutOrders	Private 🔐	🛑
└	_submitOrder	Private 🔐	🛑
└	_safeSubmitOrder	Private 🔐	🛑
└	_transferToReserveAndStore	Private 🔐	🛑
└	_transferInputTokens	Private 🔐	🛑
└	_handleUnderSpending	Private 🔐	🛑
└	_transferFeeWithRoyalty	Private 🔐	🛑
└	_decreaseHoldingAmount	Private 🔐	🛑
└	_safeTransferAndUnwrap	Private 🔐	🛑
└	_safeTransferWithFees	Private 🔐	🛑
└	_calculateFees	Private 🔐

FeeSplitter	Implementation	Ownable, ReentrancyGuard
└	<Constructor>	Public ❗️	🛑	NO❗️
└	<Receive Ether>	External ❗️	💵	NO❗️
└	getAmountDue	Public ❗️		NO❗️
└	setRoyaltiesWeight	Public ❗️	🛑	onlyOwner
└	setShareholders	Public ❗️	🛑	onlyOwner
└	releaseToken	Public ❗️	🛑	nonReentrant
└	releaseTokens	External ❗️	🛑	NO❗️
└	releaseETH	External ❗️	🛑	nonReentrant
└	sendFees	External ❗️	🛑	nonReentrant
└	sendFeesWithRoyalties	External ❗️	🛑	nonReentrant
└	updateShareholder	External ❗️	🛑	onlyOwner
└	totalShares	External ❗️		NO❗️
└	totalReleased	External ❗️		NO❗️
└	shares	External ❗️		NO❗️
└	released	External ❗️		NO❗️
└	findShareholder	External ❗️		NO❗️
└	_sendFees	Private 🔐	🛑
└	_addShares	Private 🔐	🛑
└	_releaseToken	Private 🔐	🛑
└	_addShareholder	Private 🔐	🛑
└	_computeShareCount	Private 🔐

NestedAsset	Implementation	ERC721Enumerable, Ownable
└	<Constructor>	Public ❗️	🛑	ERC721
└	tokenURI	Public ❗️		NO❗️
└	originalOwner	Public ❗️		NO❗️
└	mint	Public ❗️	🛑	onlyFactory
└	mintWithMetadata	External ❗️	🛑	onlyFactory
└	backfillTokenURI	External ❗️	🛑	onlyFactory onlyTokenOwner
└	burn	External ❗️	🛑	onlyFactory onlyTokenOwner
└	setFactory	External ❗️	🛑	onlyOwner
└	removeFactory	External ❗️	🛑	onlyOwner
└	_setTokenURI	Internal 🔒	🛑

Legend

Symbol	Meaning
🛑	Function can modify state
💵	Function is payable

#0 - adrien-supizet
2021-11-23T16:52:45Z

Affected contracts (see ):

NestedRecords:

createRecord can be internal if I'm not mistaken, it does not seem used by the Factory, nor the frontend

NestedFactory ☑️

FeeSplitter:

setShareholders and setRoyaltiesWeight can be external, and we could duplicate the code in the calls from the constructor. Unnecessary as these functions will be very rarely called.

NestedAsset:

mint can be external if we duplicate the code in mintWithMetadata and use the same internal function _mint

#1 - maximebrugel
2021-12-20T14:42:53Z

Note :

createRecord has been removed.
setRoyaltiesWeight & setShareholders can't be external since they're called from the constructor.

The resolution will be focus on the mint function.

Findings Information

🌟 Selected for report: GiveMeTestEther

Also found by: WatchPug, defsec, pauliax

Labels

bug

G (Gas Optimization)

sponsor confirmed

Awards

19.3212 USDC - $19.32

External Links

Link to GitHub issue

Handle

GiveMeTestEther

Vulnerability details

Impact

FeeSplitter.so: totalWeights is the sum of shareholder weights and royaltiesWeight, therefore a subtraction of a shareholder weight or royaltiesWeight can be done unchecked because we can't underflow and save gas.

Proof of Concept

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L143

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L169

Tools Used

Manual Analysis

Recommended Mitigation Steps

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L143 can be rewritten as:

function sendFees(IERC20 _token, uint256 _amount) external nonReentrant {
    _sendFees(_token, _amount, unchecked {totalWeights - royaltiesWeight});
}

https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L169 can be written as unchecked { _totalWeights -= shareholders[_accountIndex].weight; }

#0 - maximebrugel
2021-11-16T16:42:38Z

The recommended mitigation can't be implemented, because the unchecked block can't be used as an expression :

Please check => https://github.com/ethereum/solidity/issues/10698

However, it can be implemented that way :

function sendFees(IERC20 _token, uint256 _amount) external nonReentrant {
    uint256 weights;
    unchecked {
        weights = totalWeights - royaltiesWeight;
    }
    _sendFees(_token, _amount, weights);
}

Before :

After :

#1 - adrien-supizet
2021-11-17T12:56:50Z

Issue acknowledged. Still need to decide if we want to implement it, regarding the maintainability of the protocol.

#2 - maximebrugel
2021-11-22T16:30:03Z

As mentioned in #214, we can add unchecked in :

if (_inputTokenAmounts[i] - amountSpent > 0) {
    _transferFeeWithRoyalty(_inputTokenAmounts[i] - amountSpent, _inputToken, _nftId); //unchecked
}

#3 - maximebrugel
2021-12-02T13:49:19Z

As mentioned in #214, we can add unchecked in :

if (_inputTokenAmounts[i] - amountSpent > 0) {
    _transferFeeWithRoyalty(_inputTokenAmounts[i] - amountSpent, _inputToken, _nftId); //unchecked
}

Resolved by https://github.com/NestedFinance/nested-core-lego/pull/28

Nested Finance contest - GiveMeTestEther's results

The one-stop Defi app to build, manage and monetize your portfolio.

General Information

Findings Distribution

Researcher Performance

External Links

QA Report - $389.97

Gas Report - $172.88

QA Report - $389.97

Gas Report - $172.88

🌟 [L-04] FeeSplitter:` totalWeights` can be set to 0 by `onlyOwner` - $389.97

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

[G-06] Require reasons strings can be shortened - $10.43

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - maximebrugel2021-11-18T14:05:25Z

🌟 [G-07] unchecked { ++i } is more gas efficient than i++ for loops - $47.71

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - adrien-supizet2021-11-18T12:01:44Z

🌟 [G-10] More gas efficient calculation of weights - $47.71

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - maximebrugel2021-11-17T16:36:03Z

🌟 [G-11] Mix of external and public function visibility with the same access modifier - $47.71

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Tools Used

Recommended Mitigation Steps

#0 - adrien-supizet2021-11-23T16:52:45Z

#1 - maximebrugel2021-12-20T14:42:53Z

🌟 [G-15] Subtraction from` totalWeights` can be done unchecked to save gas - $19.32

Findings Information

Labels

Awards

External Links

Handle

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - maximebrugel
2021-11-18T14:05:25Z

#0 - adrien-supizet
2021-11-18T12:01:44Z

#0 - maximebrugel
2021-11-17T16:36:03Z

#0 - adrien-supizet
2021-11-23T16:52:45Z

#1 - maximebrugel
2021-12-20T14:42:53Z

#0 - maximebrugel
2021-11-16T16:42:38Z

#1 - adrien-supizet
2021-11-17T12:56:50Z

#2 - maximebrugel
2021-11-22T16:30:03Z

#3 - maximebrugel
2021-12-02T13:49:19Z