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

can withdraw shares on behalf of anyone #215

Open
code423n4 opened this issue Nov 10, 2021 · 2 comments
Open

can withdraw shares on behalf of anyone #215

code423n4 opened this issue Nov 10, 2021 · 2 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

cmichel

Vulnerability details

The BasicSale.withdrawShareForMember function allows withdrawing the shares of other users.
While the tokens are sent to the correct address, this can lead to issues with smart contracts that might rely on claiming the tokens themselves.

As an example, suppose the member address corresponds to a smart contract that has a function of the following form:

function claimAndDoSomething() {
    uint256 claimedAmount = mainToken.balanceOf(address(this));
    contract.withdrawShare();
    claimedAmount = mainToken.balanceOf(address(this)) - claimedAmount;
    mainToken.transfer(externalWallet, claimedAmount);
}

Impact

If the contract has no other functions to transfer out funds, they may be locked forever in this contract.

Recommended Mitigation Steps

Do not allow users to withdraw on behalf of other users.

@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 Nov 10, 2021
code423n4 added a commit that referenced this issue Nov 10, 2021
@chickenpie347 chickenpie347 added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Jan 4, 2022
@chickenpie347
Copy link
Collaborator

This function is not intended to be used by smart contracts, and if indeed smart contracts are created to be used, I am sure they would follow the intricacies needed.

@0xean
Copy link
Collaborator

0xean commented Jan 8, 2022

I am not clear on what the benefit of allowing someone to withdraw on behalf of someone else actually is here. Will leave the issue as low risk

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

3 participants