code-423n4 / 2022-05-aura-findings

0 stars 1 forks source link

claim() in AuraMerkleDrop every address can withdraw one times and if there was two leaf for one address, user can't withdraw both of them #320

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraMerkleDrop.sol#L114-L143

Vulnerability details

Impact

If there were two leaf with on address then that address can't withdraw both of them. the logic in the claim() keeps track of which user withdrew his tokens instead it should keep track of which leaf has been used.

Proof of Concept

This is claim() code in AuraMerkleDrop:

    function claim(
        bytes32[] calldata _proof,
        uint256 _amount,
        bool _lock
    ) public returns (bool) {
        require(merkleRoot != bytes32(0), "!root");
        require(block.timestamp > startTime, "!started");
        require(block.timestamp < expiryTime, "!active");
        require(_amount > 0, "!amount");
        require(hasClaimed[msg.sender] == false, "already claimed");

        bytes32 leaf = keccak256(abi.encodePacked(msg.sender, _amount));
        require(MerkleProof.verify(_proof, merkleRoot, leaf), "invalid proof");

        hasClaimed[msg.sender] = true;

        if (_lock) {
            aura.safeApprove(address(auraLocker), 0);
            aura.safeApprove(address(auraLocker), _amount);
            auraLocker.lock(msg.sender, _amount);
        } else {
            // If there is an address for auraLocker, and not locking, apply 20% penalty
            uint256 penalty = address(auraLocker) == address(0) ? 0 : (_amount * 2) / 10;
            pendingPenalty += penalty;
            aura.safeTransfer(msg.sender, _amount - penalty);
        }

        emit Claimed(msg.sender, _amount, _lock);
        return true;
    }

As you can see, when user shows proof for one leaf the code mark msg.sender in hasClaimed, so one address can't claim multiple times if his address is in multiple leaf.

Tools Used

VIM

Recommended Mitigation Steps

contract logic should mark leaf identity for withdraw events in hasClaimed instead of msg.address

0xMaharishi commented 2 years ago

Each user has one leaf

dmvt commented 2 years ago

Invalid.