Nested Finance contest - PierrickGT's results

Handle

PierrickGT

Vulnerability details

Impact

In setMaxAllowance, safeApprove is used to increase allowance. As stated in the following Pull Request, safeApprove has been deprecated in favor of safeIncreaseAllowance and safeDecreaseAllowance.

https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2268/files

This is because safeApprove shouldn't check for allowance, as explained in the issue below:

https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2219

approve is actually vulnerable to a sandwich attack as explained in the following document and this check for allowance doesn't actually avoid it.

https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit

Proof of Concept

safeIncreaseAllowance should be used to increase allowance and safeDecreaseAllowance to decrease allowance to 0. We can also gain in code clarity by refactoring the if else statement and calling _token.safeIncreaseAllowance(_spender, type(uint256).max); only once.

Recommended Mitigation Steps

The following changes are recommended.

function setMaxAllowance(IERC20 _token, address _spender) internal {
    uint256 _currentAllowance = _token.allowance(address(this), _spender);

    if (_currentAllowance != type(uint256).max) {
        // Decrease to 0 first for tokens mitigating the race condition
        _token.safeDecreaseAllowance(_spender, _currentAllowance);
    }

    _token.safeIncreaseAllowance(_spender, type(uint256).max);
}

Handle

PierrickGT

Vulnerability details

Impact

In _calculateFees, the parameter _user is passed to the function but never used inside of it.

Proof of Concept

Remove the _user parameter in _calculateFees to gain in code clarity. By removing this parameter, we also need to call the function without passing any address at the following lines:

• L228
• L298
• L354
• L548

Recommended Mitigation Steps

The following change is recommended.

function _calculateFees(uint256 _amount) private view returns (uint256) {
    return _amount / 100;
}

Handle

PierrickGT

Vulnerability details

Impact

In _transferToReserveAndStore, _token is casted 3 times to IERC20 and reserve is loaded and casted to address 4 times. We can simplify the function and save on gas.

Proof of Concept

_token should be passed to the function as an IERC20, this way we avoid to cast it 3 times. reserve should be stored in a variable to avoid 3 unnecessary sloads and casting.

Recommended Mitigation Steps

The following changes are recommended.

    function _transferToReserveAndStore(
        IERC20 _token,
        uint256 _amount,
        uint256 _nftId
    ) private {
        address reserveAddress = address(reserve);
        uint256 balanceReserveBefore = _token.balanceOf(reserveAddress);

        // Send output to reserve
        _token.safeTransfer(reserveAddress, _amount);

        uint256 balanceReserveAfter = _token.balanceOf(reserveAddress);

        nestedRecords.store(_nftId, address(_token), balanceReserveAfter - balanceReserveBefore, reserveAddress);
    }

After this subsequent change, _outputToken will need to be casted to IERC20 on L386.

_transferToReserveAndStore(IERC20(_outputToken), amounts[0], _nftId);

And no need to cast _outputToken anymore on L357.

_transferToReserveAndStore(_outputToken, amountBought - feesAmount, _nftId);

Handle

PierrickGT

Vulnerability details

Impact

In deleteAsset, tokens is declared once in the function and then a second time in the function freeToken.

Proof of Concept

The freeToken function being used only in deleteAsset, the code from this function can be moved to deleteAsset and the function removed. This way, we don't have to pass tokens to the freeToken function and we avoid declaring it here a second time.

Recommended Mitigation Steps

The following change is recommended.

function deleteAsset(uint256 _nftId, uint256 _tokenIndex) public onlyFactory {
    address[] storage tokens = records[_nftId].tokens;
    address token = tokens[_tokenIndex];
    Holding memory holding = records[_nftId].holdings[token];

    require(holding.isActive, "NestedRecords: HOLDING_INACTIVE");

    delete records[_nftId].holdings[token];
    tokens[_tokenIndex] = tokens[tokens.length - 1];
    tokens.pop();
}

Handle

PierrickGT

Vulnerability details

Impact

In _submitOrder, we load in memory the return parameter tokens from decodeDataAndRequire but this variable isn't used afterward.

Proof of Concept

Avoid loading tokens from decodeDataAndRequire to save on gas and gain in code clarity. https://github.com/code-423n4/2021-11-nested/blob/cbd39fe7d76ed8c84eb767a5f3b6eba83e034656/contracts/NestedFactory.sol#L379

Recommended Mitigation Steps

The following change is recommended.

(uint256[] memory amounts,) = OperatorHelpers.decodeDataAndRequire(
    data,
    _inputToken,
    _outputToken
);

Handle

PierrickGT

Vulnerability details

Impact

In setReserve function, the param _reserve isn't checked against address zero. If address zero is mistakenly passed to the function, the transaction would still be successful and it could cause undesired bugs when interacting with reserve, it would also be a waste of gas.

Proof of Concept

By checking the address passed against address zero, we avoid any future bugs and we can revert earlier during the code execution to save on gas, it also provides a clear error message.

Recommended Mitigation Steps

Add the following require statement to check for address zero on line 90, before the other require.

require(address(_reserve) != address(0), "NestedFactory::setReserve: Invalid reserve address");