Nested Finance contest - fatima_naz's results

Handle

fatima_naz

Vulnerability details

Impact

In line - 20 , 29 and 30. ERC20 transfer and transferFrom are used. The race condition that happens the most on the network today is the race condition in the ERC20 token standard. The ERC20 token standard includes a function called 'approve' which allows an address to approve another address to spend tokens on their behalf. Assume that Alice has approved Eve to spend n of her tokens, then Alice decides to change Eve's approval to m tokens. Alice submits a function call to approve with the value n for Eve. Eve runs a Ethereum node so knows that Alice is going to change her approval to m. Eve then submits a tranferFrom request sending n of Alice's tokens to herself, but gives it a much higher gas price than Alice's transaction. The transferFrom executes first so gives Eve n tokens and sets Eve's approval to zero. Then Alice's transaction executes and sets Eve's approval to m. Eve then sends those m tokens to herself as well. Thus Eve gets n + m tokens even thought she should have gotten at most max(n,m).

Proof of Concept

https://swcregistry.io/docs/SWC-114 https://medium.com/coinmonks/solidity-transaction-ordering-attacks-1193a014884e

Tools Used

Recommended Mitigation Steps

use SafeERC20 - Library that brings an abstract layer above the ERC20 standard interface providing a way to call its methods safely by checking pre and post-conditions.

Add these 2 lines- import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol"; using SafeERC20 for IERC20

Replace transferFrom with SafeTransferFrom Replace transfer with SafeTransfer

Handle

fatima_naz

Vulnerability details

Impact

In NestedFactory.sol line number - 299 assert(amountSpent <= _inputTokenAmount - feesAmount); If the condition fails then assert(false) compiles to 0xfe, which is an invalid opcode, using up all remaining gas, and reverting all changes. So we should use require because require(false) compiles to 0xfd which is the REVERT opcode, meaning it will refund the remaining gas. The opcode can also return a value (useful for debugging)

Proof of Concept

Gas efficiency assert(false) compiles to 0xfe, which is an invalid opcode, using up all remaining gas, and reverting all changes.

require(false) compiles to 0xfd which is the REVERT opcode, meaning it will refund the remaining gas.

Links - https://medium.com/blockchannel/the-use-of-revert-assert-and-require-in-solidity-and-the-new-revert-opcode-in-the-evm-1a3a7990e06e https://ethereum.stackexchange.com/questions/15166/difference-between-require-and-assert-and-the-difference-between-revert-and-thro

Tools Used

Recommended Mitigation Steps

Change assert with require- require(amountSpent <= _inputTokenAmount - feesAmount); we can send some message if condition fails- assert(amountSpent <= _inputTokenAmount - feesAmount , "amountSpent is greater than required") ;

Handle

fatima_naz

Vulnerability details

Impact

in function removeOperator in NestedFactory.sol function removeOperator(bytes32 operator) external override onlyOwner { uint256 i = 0; while (operators[i] != operator) { i++; } require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator"); delete operators[i]; }

In the require condition i>0 but if the first element of operators i.e. operators[0] is equal to the operator the while loop will break just then. so i is 0 but require condition checks for i>0 so it will show that "NestedFactory::removeOperator: Cant remove non-existent operator" but actually the operator exist and it needs to be removed.

Proof of Concept

Tools Used

Recommended Mitigation Steps

change the condition in require as i>=0

Handle

fatima_naz

Vulnerability details

Impact

In function removeOperator - function removeOperator(bytes32 operator) external override onlyOwner { uint256 i = 0; while (operators[i] != operator) { i++; } require(i > 0, "NestedFactory::removeOperator: Cant remove non-existent operator"); delete operators[i]; }

In the while condition it is not checked that i<operators.length so if the required operator does not exist in the array then the condition operators[i] != operator will always be true and variable i will always increase. So once it is equal to the length of array it will try to access that index but that index is out of bound to it will throw an exception.

Proof of Concept

https://github.com/TokenMarketNet/smart-contracts/issues/101 https://jeancvllr.medium.com/solidity-tutorial-all-about-array-efdff4613694

Tools Used

Recommended Mitigation Steps

we need to add a check in while loop that i<operators.length