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

Incorrect event parameters in transferFrom function #36

Open
code423n4 opened this issue Sep 8, 2021 · 2 comments
Open

Incorrect event parameters in transferFrom function #36

code423n4 opened this issue Sep 8, 2021 · 2 comments
Labels
1 (Low Risk) bug Something isn't working disagree with severity duplicate This issue or pull request already exists

Comments

@code423n4
Copy link
Contributor

Handle

JMukesh

Vulnerability details

Impact

different parameter are being set in Approval event in transferFrom()

function transferFrom(
address from,
address to,
uint256 amount
) external override returns (bool) {
(bool success, uint256 newAllowance) =
proxy.nTokenTransferFrom(currencyId, msg.sender, from, to, amount);

    // Emit transfer events here so they come from the correct contract
    emit Transfer(from, to, amount);

// here first parameter should be owner and second should be spender
// as mentioned in ntokenErc20.sol that is :
// event Approval(address indexed owner, // address indexed spender, uint256 amount);

    emit Approval(msg.sender, from, newAllowance);

    return success;
}

This error may negatively impact
off-chain tools that are monitoring critical transfer events of the token.

Proof of Concept

https://github.com/code-423n4/2021-08-notional/blob/4b51b0de2b448e4d36809781c097c7bc373312e9/contracts/external/adapters/nTokenERC20Proxy.sol#L100

Tools Used

manual review

Recommended Mitigation Step

@code423n4 code423n4 added 3 (High Risk) bug Something isn't working labels Sep 8, 2021
code423n4 added a commit that referenced this issue Sep 8, 2021
@jeffywu
Copy link
Collaborator

jeffywu commented Sep 11, 2021

Duplicate #55, dispute the categorization. This should be Low Risk.

@jeffywu jeffywu added disagree with severity duplicate This issue or pull request already exists labels Sep 11, 2021
@ghoul-sol
Copy link
Collaborator

External services might be affected but it's not clear how significant it would be. Most of the time events are not critical. Making this low risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) bug Something isn't working disagree with severity duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants