Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #52

Open
code423n4 opened this issue Feb 12, 2022 · 3 comments
Open

QA Report #52

code423n4 opened this issue Feb 12, 2022 · 3 comments
Assignees
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

[N1] Unused imports

The following source units are imported but not referenced in the contract:

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/operators/Flat/FlatOperator.sol#L4-L4

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/operators/Flat/IFlatOperator.sol#L4-L4

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

Recommendation

Check all imports and remove all unused/unreferenced and unnecessary imports.

[N2] Using public to generate the getter function can make the code simpler and cleaner

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/operators/ZeroEx/ZeroExStorage.sol#L7-L19

contract ZeroExStorage is Ownable {
    address private _swapTarget;

    /// @notice Returns the address of 0x swaptarget
    function swapTarget() external view returns (address) {
        return _swapTarget;
    }

    /// @notice Update the address of 0x swaptarget
    function updatesSwapTarget(address swapTargetValue) external onlyOwner {
        _swapTarget = swapTargetValue;
    }
}

Can be changed to:

contract ZeroExStorage is Ownable {
    address public swapTarget;

    /// @notice Update the address of 0x swaptarget
    function updatesSwapTarget(address swapTargetValue) external onlyOwner {
        swapTarget = swapTargetValue;
    }
}

[N3] Inconsistent use of _msgSender()

Direct use of msg.sender vs internal call of _msgSender().

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/operators/ZeroEx/ZeroExOperator.sol#L17-L17

ZeroExStorage(operatorStorage).transferOwnership(msg.sender);

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/NestedReserve.sol#L30-L30

_token.safeTransfer(msg.sender, _amount);

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/abstracts/OwnableFactoryHandler.sol#L21-L21

require(supportedFactories[msg.sender], "OFH: FORBIDDEN");

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/FeeSplitter.sol#L103-L103

require(msg.sender == weth, "FS: ETH_SENDER_NOT_WETH");

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/FeeSplitter.sol#L166-L168

amount = _releaseToken(_msgSender(), _tokens[i]);
_tokens[i].safeTransfer(_msgSender(), amount);
emit PaymentReleased(_msgSender(), address(_tokens[i]), amount);

https://github.com/code-423n4/2022-02-nested/blob/879bae87b1987d6810f25c1082e5bf664390ae7f/contracts/FeeSplitter.sol#L199-L199

_token.safeTransferFrom(_msgSender(), address(this), _amount);

Recommendation

Consider replacing _msgSender() with msg.sender for consistency.

@code423n4 code423n4 added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax bug Something isn't working labels Feb 12, 2022
code423n4 added a commit that referenced this issue Feb 12, 2022
@maximebrugel
Copy link
Collaborator

"Unused imports" (Confirmed)

"Using public to generate the getter function can make the code simpler and cleaner" (Confirmed)

"Inconsistent use of _msgSender()" (Disputed)

The one with msg.sender are used when meta transactions are not supported (sender is WETH, Nested Factory,...)

@maximebrugel maximebrugel added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 15, 2022
@maximebrugel maximebrugel self-assigned this Feb 16, 2022
@harleythedogC4
Copy link
Collaborator

My personal judgements:

  1. "[N1] Unused imports". Valid and non-critical.
  2. "[N2] Using public to generate the getter function can make the code simpler and cleaner". Valid and non-critical.
  3. "[N3] Inconsistent use of _msgSender()". As sponsor describes. Invalid.

@harleythedogC4
Copy link
Collaborator

Now, here is the methodology I used for calculating a score for each QA report. I first assigned each submission to be either non-critical (1 point), very-low-critical (5 points) or low-critical (10 points), depending on how severe/useful the issue is. The score of a QA report is the sum of these points, divided by the maximum number of points achieved by a QA report. This maximum number was 26 points, achieved by #66.

The number of points achieved by this report is 2 points.
Thus the final score of this QA report is (2/26)*100 = 8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants