Nested Finance contest - defsec's results

Handle

defsec

Vulnerability details

Impact

The current ownership transfer process involves the current owner calling NestedReserve.transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately

for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert

Proof of Concept

1. Navigate to "https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedReserve.sol#L10"
2. The contracts have many onlyOwner function.
3. The contract is inherited from the Ownable which includes transferOwnership.

Tools Used

None

Recommended Mitigation Steps

Implement zero address check and Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

Handle

defsec

Vulnerability details

Impact

This does not directly impact the smart contract in anyway besides cost. This is a gas optimization to reduce cost of smart contract. Calling each function, we can see that the public function uses 496 gas, while the external function uses only 261.

Proof of Concept

According to Slither Analyzer documentation (https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external), there are functions in the contract that are never called. These functions should be declared as external in order to save gas.

Slither Detector:

external-function:

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

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

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

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

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

Tools Used

Slither

Recommended Mitigation Steps

1. Get Smart Contracts from the Repository.
2. Create a python virtual environment with a stable python version.
3. Install Slither Analyzer on the python VEM.
4. Run Slither against all contracts.
5. Define public functions as an external for the gas optimization.

Handle

defsec

Vulnerability details

Impact

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

Proof of Concept

1. Navigate to the following contracts.

"https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedFactory.sol#L481"

"" https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedFactory.sol#L84 ""

Tools Used

Code Review

Recommended Mitigation Steps

Use "!=0" instead of ">0" for the gas optimization.

Handle

defsec

Vulnerability details

Impact

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

Proof of Concept

Example Code Location

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

Tools Used

Code Review

Recommended Mitigation Steps

Consider applying 'unchecked' keyword where overflows/underflows are not possible.

Handle

defsec

Vulnerability details

Impact

The use of _msgSender() when there is no implementation of a meta transaction mechanism that uses it, such as EIP-2771, very slightly increases gas consumption.

Proof of Concept

1. Navigate to the following contracts.

"https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/FeeSplitter.sol#L135" "https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedFactory.sol#L111" "https://github.com/code-423n4/2021-11-nested/blob/5d113967cdf7c9ee29802e1ecb176c656386fe9b/contracts/NestedReserve.sol#L32"

Tools Used

Code review

Recommended Mitigation Steps

Replace _msgSender() with msg.sender if there is no mechanism to support meta-transactions like EIP-2771 implemented.

Handle

defsec

Vulnerability details

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

Instances include:

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

Tools Used

Code Review

Recommended Mitigation Steps

Here you could cache uint len = _moduleAddresses.length; and then change the stop criteria to use len instead _moduleAddresses.length to save gas (_moduleAddresses is on memory)