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

Claim SHER on behalf of others #271

Open
code423n4 opened this issue Jan 26, 2022 · 2 comments
Open

Claim SHER on behalf of others #271

code423n4 opened this issue Jan 26, 2022 · 2 comments
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) help wanted Extra attention is needed

Comments

@code423n4
Copy link
Contributor

Handle

pauliax

Vulnerability details

Impact

In SherClaim consider letting claim SHER tokens on behalf of other users. function add does not have any restrictions on the caller, thus anyone can increase the userClaims of any _user. On the other hand, the claim function only allows msg.sender to withdraw userClaims. If the _user is a smart contract that does not implement the claim functionality, their SHER tokens will be left stuck in the contract. While I expect this is not very likely to happen, I see no harm if you add a claim function with a _user parameter so anyone can help others to claim.

Also, I think it would be nice if ISherClaim interface contained function claim() so that integrators can copy the interface if necessary.

One more thing, you can consider adding a token sweep function to this contract also, and introduce a claim deadline, so an admin can rescue unclaimed or accidentally sent tokens later. But it depends on the intentions, maybe you do not want to enforce users and are okay if some SHER will be left there forever.

Recommended Mitigation Steps

An example implementation:

  function claim(address _user) external {
    // Only allow claim calls if claim period is active
    if (active() == false) revert InvalidState();

    // How much SHER the user will receive
    uint256 amount = userClaims[_user];
    // Dont proceed if it's 0 SHER
    if (amount == 0) revert InvalidAmount();
    // If it is not 0, make sure it's 0 next time the user calls this function
    delete userClaims[_user];

    // Transfer SHER to user
    sher.safeTransfer(_user, amount);

    // Emit event about the SHER claim
    emit Claim(msg.sender, _user, amount);
  }
@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 Jan 26, 2022
code423n4 added a commit that referenced this issue Jan 26, 2022
@Evert0x Evert0x added disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) help wanted Extra attention is needed labels Feb 9, 2022
@Evert0x
Copy link
Collaborator

Evert0x commented Feb 9, 2022

Informational

@jack-the-pug
Copy link
Collaborator

A enhancement, not an issue.

@jack-the-pug jack-the-pug added invalid This doesn't seem right 0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation and removed invalid This doesn't seem right 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments labels Mar 26, 2022
@jack-the-pug jack-the-pug reopened this Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants