c4-bot-10 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L138-L145

Vulnerability details

Impact

A user can get his neuron airdrop withou calling the claim function. As a result, if his off-chain service needs to account claimed airdrops using the emited events, they will not track the airdrop claimed by the user

Proof of Concept

    function testClamAirdropWithoutEmitingEvent() public {
        address receiver = vm.addr(3);
        address[] memory recipients = new address[](1);
        recipients[0] = receiver;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1_000 * 10 ** 18;
        _neuronContract.setupAirdrop(recipients, amounts);

        vm.startPrank(receiver);
        uint256 balanceBefore = _neuronContract.balanceOf(receiver);
        _neuronContract.transferFrom(_treasuryAddress, receiver, 1_000 * 10 ** 18);
        uint256 balanceAfter = _neuronContract.balanceOf(receiver);
        vm.stopPrank();

        assertEq(balanceAfter - balanceBefore, 1000 * 10 **18);
    }

Output traces

Running 1 test for test/Neuron.t.sol:NeuronTest
[PASS] testClamAirdropWithoutEmitingEvent() (gas: 57408)
Traces:
  [61621] NeuronTest::testClamAirdropWithoutEmitingEvent()
    ├─ [0] VM::addr(3) [staticcall]
    │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    ├─ [29730] Neuron::setupAirdrop([0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69], [1000000000000000000000 [1e21]])
    │   ├─ emit Approval(owner: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, spender: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, value: 1000000000000000000000 [1e21])
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   └─ ← ()
    ├─ [2586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 0
    ├─ [24519] Neuron::transferFrom(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 1000000000000000000000 [1e21])
    │   ├─ emit Approval(owner: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, spender: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, value: 0)
    │   ├─ emit Transfer(from: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, value: 1000000000000000000000 [1e21])
    │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    ├─ [586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 1000000000000000000000 [1e21]
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.82ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

I would not implement the airdrop as allowing users to move funds from the treasury. I would instead make a mapping for each user storing how much they can claim. And then the neuron contract had approved the uint256 max from the treasury and transfer funds from treasury to users that would call the claim function.

Assessed type

Error

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

raymondfam commented 8 months ago

event will be emitted as intended.

c4-pre-sort commented 8 months ago

raymondfam marked the issue as primary issue

c4-judge commented 8 months ago

HickupHH3 marked the issue as unsatisfactory: Invalid

Draiakoo commented 8 months ago

The only events that has been emitted in the PoC (as it can be checked in the output traces) are the Approval and Transfer. But the event that is intended to be emitted when someone claims the airdrop is TokensClaimed which is here is not emitted because the airdrop can be claimed avoiding to call this function, just by transfering the funds from the treasury to your own account. If the offchain server depends on this event for accounting the airdrops claimed, the accounting will freak because people can claim them without calling the function and without emiting the TokensClaimed event.