hats-finance / OLD-Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784

MIT License
0 stars 0 forks source link

consensusDelegate allows for an illegitimate receipt to go through #1

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x44ef391a519126fc96de190f4f9c355dea43b4620dcc6d843817415eaaac30cd Severity: medium

Description: As we know receiptId starts incrementing from nextReceiptId which is set 4294967296:

Whenever delegating function delegate is invoked which creates the receiptId in the following way:

    function delegate(StakingAddress to, uint128 amount) public onlyOwner returns (uint64) {
        require(amount < type(uint128).max, ">MaxUint128");
        require(amount > 0, "ZeroDelegate");
=>        uint64 receiptId = nextReceiptId++;
        Subcall.consensusDelegate(to, amount, receiptId);

This means that the first receiptId will be 4294967296 + 1:

After this consensusDelegate is entered and performs the following check:

    function consensusDelegate(
        StakingAddress to,
        uint128 amount,
        uint64 receiptId
    ) internal returns (bytes memory data) {

=>        if (receiptId < 4294967296) revert InvalidReceiptId();

This however is incorrect since the first ever existing receiptId starts at 4294967297, but instead this check allows 4294967296 to go through.

Ultimately a non-existing receiptId that is lower than the starting point is allowed to go through.

Note that this vulnerability also exists inside consensusUndelegate but due to the similarity this is not reported as separate issues

Recommendation

increment the number

-       if (receiptId < 4294967296) revert InvalidReceiptId();
+       if (receiptId <= 4294967296) revert InvalidReceiptId();

This way anything which is lower than the first receiptId will revert.

0xRizwan commented 2 months ago

Ultimately a non-existing receiptId that is lower than the starting point is allowed to go through.

On first delegate via delegate() function, the receiptId would be 4294967296 + 1 then this receiptId would be checked in Subcall.consensusDelegate(to, amount, receiptId); and it would be passed. The above issue wont occur in practical.