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 parameter used in emit #105

Open
code423n4 opened this issue Nov 7, 2021 · 1 comment
Open

Incorrect event parameter used in emit #105

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

Comments

@code423n4
Copy link
Contributor

Handle

Reigada

Vulnerability details

Impact

In the contracts AirdropDistribution and InvestorDistribution the event Vested is emitted with a wrong amount. The amount that should be used are:

  1. claimable_not_yet_vested instead of claimable for the claim() function
  2. claimable_not_yet_vested instead of _value for claimExact() function

Proof of Concept

function claim() external nonReentrant {
    require(msg.sender != address(0));
    require(validated[msg.sender] == 1, "Address not validated to claim.");
    require(airdrop[msg.sender].amount != 0);
    
    uint256 avail = _available_supply();
    require(avail > 0, "Nothing claimable (yet?)");

    uint256 claimable = avail * airdrop[msg.sender].fraction / 10**18;
    assert(claimable > 0);
    if (airdrop[msg.sender].claimed != 0) {
        claimable -= airdrop[msg.sender].claimed;
    }

    assert(airdrop[msg.sender].amount - claimable != 0);

    airdrop[msg.sender].amount -= claimable;
    airdrop[msg.sender].claimed += claimable;

    uint256 claimable_to_send = claimable * 3 / 10;         //30% released instantly
    mainToken.transfer(msg.sender, claimable_to_send);
    uint256 claimable_not_yet_vested = claimable - claimable_to_send; 
    vestLock.vest(msg.sender, claimable_not_yet_vested, 0); //70% locked in vesting contract

    emit Vested(msg.sender, claimable, block.timestamp);
}


//Allow users to claim a specific amount instead of the entire amount
function claimExact(uint256 _value) external nonReentrant {
    require(msg.sender != address(0));
    require(airdrop[msg.sender].amount != 0);
    
    uint256 avail = _available_supply();
    uint256 claimable = avail * airdrop[msg.sender].fraction / 10**18; //
    if (airdrop[msg.sender].claimed != 0){
        claimable -= airdrop[msg.sender].claimed;
    }

    require(airdrop[msg.sender].amount >= claimable);
    require(_value <= claimable);
    airdrop[msg.sender].amount -= _value;
    airdrop[msg.sender].claimed += _value;

    uint256 claimable_to_send = _value * 3 / 10;
    mainToken.transfer(msg.sender, claimable_to_send);
    uint256 claimable_not_yet_vested = _value - claimable_to_send;
    vestLock.vest(msg.sender, claimable_not_yet_vested, 0);

    emit Vested(msg.sender, _value, block.timestamp); 
}

Tools Used

Manual testing

Recommended Mitigation Steps

  1. Recommend using claimable_not_yet_vested instead of claimable in the claim() function Vested event
  2. Recommend using claimable_not_yet_vested instead of _value in the claimExact() function Vested event
@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 7, 2021
code423n4 added a commit that referenced this issue Nov 7, 2021
@veloboot
Copy link
Collaborator

This may be a semantic issue since, in the latest version, the amount claimed is now emitted with a Claimed event rather than the vested amount that already has its own distinct event emitted from the Vesting contract. However, this was a bug in the code that was presented for audit. The recommended mitigation steps will not be followed, however.

@chickenpie347 chickenpie347 added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons and removed sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Jan 3, 2022
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