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

Inconsistency in fee distribution #41

Open
code423n4 opened this issue Dec 16, 2021 · 0 comments
Open

Inconsistency in fee distribution #41

code423n4 opened this issue Dec 16, 2021 · 0 comments
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Handle

csanuragjain

Vulnerability details

Impact

Inconsistent fee disbursal

Proof of Concept

  1. Navigate to contract https://github.com/code-423n4/2021-12-nftx/blob/main/nftx-protocol-v2/contracts/solidity/NFTXSimpleFeeDistributor.sol

  2. Lets see distribute function and assume there are 5 feeReceivers

  3. Assume that distribution to 4 feeReceivers was success and loop is on last feeReceivers

  function distribute(uint256 vaultId) external override virtual nonReentrant {
    ...

    for (uint256 i = 0; i < length; i++) {
      FeeReceiver memory _feeReceiver = feeReceivers[i];
      ...
      if (!complete) {
        leftover = amountToSend;
      } else {
        leftover = 0;
      }
    }

    if (leftover > 0) {
      uint256 currentTokenBalance = IERC20Upgradeable(_vault).balanceOf(address(this));
      IERC20Upgradeable(_vault).safeTransfer(treasury, currentTokenBalance);
    }
  }
  1. Assume the transfer on last fee receiver was not success and leftover came to be 10. Also lets say currently vault balance for contract is 20.

  2. Now since leftover >0 so full vault balance is calculated and is sent to treasury

if (leftover > 0) {
      uint256 currentTokenBalance = IERC20Upgradeable(_vault).balanceOf(address(this));
      IERC20Upgradeable(_vault).safeTransfer(treasury, currentTokenBalance);
    }
  1. This is inconsistent, because if last fee receiver transfer was success then only amount 10 would have deducted from vault balance for this contract

Recommended Mitigation Steps

Change the condition

if (leftover > 0) {
      IERC20Upgradeable(_vault).safeTransfer(treasury, leftover);
    }
@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Dec 16, 2021
code423n4 added a commit that referenced this issue Dec 16, 2021
@0xKiwi 0xKiwi added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

2 participants