Nested Finance contest - pants's results

Handle

pants

Vulnerability details

Vulnerability details

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) IERC20(DAI).approve(address(uniswap), amount);

(For example line 37 ExchangeHelpers.sol)

Handle

pants

Vulnerability details

DummyRouter.sol .transfer isn't safe - with USDT can steal tokens (dummyswapToken).

We know this is a dummy contract but this should also be safe. Therefore we ask for low-risk and not high-risk.

Handle

pants

Vulnerability details

Vulnerability details

There are many for loops that follows this for-each pattern: (for example line 227 in FeeSplitter.sol or line 32 in MixinOperatorResolver.sol)

for (uint256 i = 0; i < array.length; i++) { // do something with array[i] }

In such for loops, the `array.length` is read on every iteration, instead of caching it once in a local variable and read it again using the local variable.

## Impact
Storage reads are much more expensive than reading local variables. Memory and calldata reads are a bit more expensive than reading local variables.

## Tool Used
Manual code review.

## Recommended Mitigation Steps
Read these values from storage / memory / calldata once, cache them in local variables and then read them again using the local variables. For example:

uint256 length = array.length; for (uint256 i = 0; i < length; i++) { // do something with array[i] }

Handle

pants

Vulnerability details

This impacts every for loop of the code and therefore can save a lot of gas!!! For example, FeeSplitter.sol lines 108, 125, 210, 227

Vulnerability details

There is no risk of overflow caused by increamenting the iteration index in for loops (the i++ in for (uint256 i = 0; i < numIterations; i++)).

Impact

Increaments perform overflow checks that are not necessary in this case.

Tool Used

Manual code review.

Recommended Mitigation Steps

Surround the increament expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the loop

for (uint256 i = 0; i < numIterations; i++) {
	// ...
}

to

for (uint256 i = 0; i < numIterations;) {
	// ...
	unchecked { ++i; }
}

It is a little less readable but it saves a significant amount of gas.

Handle

pants

Vulnerability details

NestedRecords line 22 - you can save storage by reordering Holding struct fields in the following way:

original: struct Holding { address token; uint256 amount; bool isActive; }

change to:

struct Holding {
    uint256 amount;
    address token;
    bool isActive;
}

Handle

pants

Vulnerability details

MixinOperatorResolver.rebuildCache (addressCache[name]), isResolverCached (addressCache[name])

You can cache the value after the first read into a local variable to save the other SLOAD and also the "out of bounds" check.

Handle

pants

Vulnerability details

Since you are using solidity version >= 0.8.0 you have additional underflow / overflow checks that takes gas cost. Here I talk about the WETHMock.withdraw where you first require(balanceOf[msg.sender] >= wad) and then compute balanceOf[msg.sender] - wad without using unchecked{...} keyword to save gas. Using this unchecked{...} keyword can help to save gas (SLOAD) of this important function.

This issue also appears at WETHMock.transferFrom, lines 75 and 78.