search

sherlock-audit / 2024-06-union-finance-update-2-judging

3 stars 2 forks source link

CFSecurity - Malicious user can steal all the funds in the `VouchFaucet` contract #96

Closed sherlock-admin4 closed 4 weeks ago

sherlock-admin4 commented 1 month ago

CFSecurity

High

Malicious user can steal all the funds in the VouchFaucet contract

Summary

A malicious user can steal all the tokens from the VouchFaucet, due to a flaw in the VouchFaucet::claimTokens() function, which is designed to rescue the tokens sent to the contract itself.

Root Cause

The VouchFaucet::claimTokens() function contains a security flaw due to insufficient access control and lack of proper validation.

Key issues:

  1. This function meant to be a used for recovering potentially lost tokens from the contract, lacks from a proper access control like an onlyOwner modifier. This omission allows any user to call it, rather than restricting access to the contract owner

  2. There is no validation of the amount parameter specified by the use, allowing him so withdraw all the tokens in the contract.

  3. The function fails to verify if the caller is entitled to the requested amount

This issues allow a malicious user to steal all the funds in the contract.

Internal pre-conditions

  1. Admin needs to set maxClaimable[token] to a number > 0.

External pre-conditions

None

Attack Path

  1. Alice accidentally sent 10e18 tokens to the contract, likely intending to to something other with them but making a mistake in the transaction.

  2. Bob, aware of a protocol flaw and anticipating such a mistake, noticed this transaction.

  3. He exploited the vulnerability by calling the VouchFaucet::claimTokens() with 10e18 tokens.

  4. This action drained all the tokens, resulting in a loss for both Alice and the protocol.

Note:

Check and run the coded PoC to understand better the vulnerability.

Impact

Due to a flaw in the VouchFaucet::claimTokens() function, anyone can steal all the tokens deposited in the contract. This vulnerability can be exploited by malicious actors, leading to significant financial losses.

PoC

  1. In order to run the test, go to the VouchFaucet.t.sol contract and replace its content with the one below:
pragma solidity ^0.8.0;

import {TestWrapper} from "../TestWrapper.sol";
import {VouchFaucet} from "union-v2-contracts/peripheral/VouchFaucet.sol";

import "forge-std/console2.sol";

contract TestVouchFaucet is TestWrapper {
    VouchFaucet public vouchFaucet;

    uint256 public TRUST_AMOUNT = 10 * UNIT;
    address BOB = address(1);
    address ALICE = address(2);

    function setUp() public {
        deployMocks();
        vouchFaucet = new VouchFaucet(address(userManagerMock), TRUST_AMOUNT);

        erc20Mock.mint(ALICE, 100 ether);

        vm.startPrank(ALICE);
        erc20Mock.approve(address(vouchFaucet), type(uint256).max);
        vm.stopPrank();
    }

    function testConfig() public {
        assertEq(vouchFaucet.USER_MANAGER(), address(userManagerMock));
        assertEq(vouchFaucet.TRUST_AMOUNT(), TRUST_AMOUNT);
        assertEq(vouchFaucet.STAKING_TOKEN(), userManagerMock.stakingToken());
    }

    function testSetMaxClaimable(address token, uint256 amount) public {
        vouchFaucet.setMaxClaimable(token, amount);
        assertEq(vouchFaucet.maxClaimable(token), amount);
    }

    function testMaliciousUserCanClaimAllTheTokensInTheContract() public {
        setUp();

        //ALICE transfers tokens to the contract
        vm.startPrank(ALICE);
        erc20Mock.transfer(address(vouchFaucet), 10 ether);

        uint256 contractBalance = erc20Mock.balanceOf(address(vouchFaucet));
        assertEq(contractBalance, 10 ether);

        vm.stopPrank();

        vm.startPrank(BOB);

        uint256 bobBalanceBefore = erc20Mock.balanceOf(BOB);
        assertEq(bobBalanceBefore, 0);

        //BOB calls claimTokens() and gets all the tokens in the contract
        vouchFaucet.claimTokens(address(erc20Mock), 10 ether);

        uint256 bobBalanceAfter = erc20Mock.balanceOf(BOB);
        assertEq(bobBalanceAfter, 10 ether);
    }

    function testCannotSetMaxClaimableNonAdmin(address token, uint256 amount) public {
        vm.prank(address(1234));
        vm.expectRevert("Ownable: caller is not the owner");
        vouchFaucet.setMaxClaimable(token, amount);
    }

    function testClaimVouch() public {
        vouchFaucet.claimVouch();
        uint256 trust = userManagerMock.trust(address(vouchFaucet), address(this));
        assertEq(trust, vouchFaucet.TRUST_AMOUNT());
    }

    function testStake() public {
        erc20Mock.mint(address(vouchFaucet), 1 * UNIT);
        assertEq(userManagerMock.balances(address(vouchFaucet)), 0);
        vouchFaucet.stake();
        assertEq(userManagerMock.balances(address(vouchFaucet)), 1 * UNIT);
    }

    function testExit() public {
        erc20Mock.mint(address(vouchFaucet), 1 * UNIT);
        assertEq(userManagerMock.balances(address(vouchFaucet)), 0);
        vouchFaucet.stake();
        assertEq(userManagerMock.balances(address(vouchFaucet)), 1 * UNIT);
        vouchFaucet.exit();
        assertEq(userManagerMock.balances(address(vouchFaucet)), 0);
    }

    function testTransferERC20(address to, uint256 amount) public {
        vm.assume(
            to != address(0) && to != address(this) && to != address(vouchFaucet) && address(vouchFaucet) != address(0)
        );

        erc20Mock.mint(address(vouchFaucet), amount);
        uint256 balBefore = erc20Mock.balanceOf(address(vouchFaucet));
        vouchFaucet.transferERC20(address(erc20Mock), to, amount);
        uint256 balAfter = erc20Mock.balanceOf(address(vouchFaucet));
        assertEq(balBefore - balAfter, amount);
        assertEq(erc20Mock.balanceOf(to), amount);
    }
}
  1. Run the coded PoC with the following command: forge test --match-test testMaliciousUserCanClaimAllTheTokensInTheContract -vvvv

Mitigation

Add a proper access control to the function like an onlyOwner modifier

sherlock-admin3 commented 1 month ago

Escalate The VouchFacet contract is not supposed to hold any funds. If you look at the contract, it is just a router to interact with the main contract. Therefore this report has no impact.

You've deleted an escalation for this issue.

c-plus-plus-equals-c-plus-one commented 1 month ago

This issue highlights the same problem as in https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/33. So these two are duplicates of each other, and should probably be merged together.

neko-nyaa commented 1 month ago

Escalate. This entire family of issue has judging problems:

96: This issue is invalid. claimTokens isn't meant to have access control. The admin sets claimable amount in setMaxClaimable, and that user can now claim. Adding onlyOwner to it breaks this function.

1: May qualify as a dupe of #33 since, while it incorrectly mentions access control, it did mention claimedTokens not being updated.

95: Same as above

107: Same as this issue (the main one) and should be invalid

sherlock-admin3 commented 1 month ago

Escalate. This entire family of issue has judging problems:

96: This issue is invalid. claimTokens isn't meant to have access control. The admin sets claimable amount in setMaxClaimable, and that user can now claim. Adding onlyOwner to it breaks this function.

1: May qualify as a dupe of #33 since, while it incorrectly mentions access control, it did mention claimedTokens not being updated.

95: Same as above

107: Same as this issue (the main one) and should be invalid

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

c-plus-plus-equals-c-plus-one commented 1 month ago

Just pasting the sponsor's initial reply from Discord here: image

cholakovvv commented 1 month ago

Escalate

https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/96: This issue is invalid. claimTokens isn't meant to have access control. The admin sets claimable amount in setMaxClaimable, and that user can now claim. Adding onlyOwner to it breaks this function.

This is not true. The claimTokens() function is not meant for users to claim tokens; instead, it is solely for rescuing tokens accidentally sent to the VouchFaucet contract. Therefore, normal users should not have access to this function.

In relation to this, it does not make sense to update claimedTokens. Therefore, all issues mentioning that should be invalidated. Issues mentioning that: #33 , #47. #51 , #52 , #63 , #97 , #99 , #100, #101, #103, #124

This is a screenshot from out private thread, where the sponsor @maxweng explains the function functionality. Maybe he can confirm here in the comments for more clarity.

Screenshot from 2024-07-21 17-08-05

sherlock-admin3 commented 1 month ago

Escalate

https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/96: This issue is invalid. claimTokens isn't meant to have access control. The admin sets claimable amount in setMaxClaimable, and that user can now claim. Adding onlyOwner to it breaks this function.

This is not true. The claimTokens() function is not meant for users to claim tokens; instead, it is solely for rescuing tokens accidentally sent to the VouchFaucet contract. Therefore, normal users should not have access to this function.

In relation to this, it does not make sense to update claimedTokens. Therefore, all issues mentioning that should be invalidated. Issues mentioning that: #33 , #47. #51 , #52 , #63 , #97 , #99 , #100, #101, #103, #124

This is a screenshot from out private thread, where the sponsor @maxweng explains the function functionality. Maybe he can confirm here in the comments for more clarity.

Screenshot from 2024-07-21 17-08-05

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

c-plus-plus-equals-c-plus-one commented 1 month ago

Escalate

96: This issue is invalid. claimTokens isn't meant to have access control. The admin sets claimable amount in setMaxClaimable, and that user can now claim. Adding onlyOwner to it breaks this function.

This is not true. The claimTokens() function is not meant for users to claim tokens; instead, it is solely for rescuing tokens accidentally sent to the VouchFaucet contract. Therefore, normal users should not have access to this function.

In relation to this, it does not make sense to update claimedTokens. Therefore, all issues mentioning that should be invalidated. Issues mentioning that: #33 , #47. #51 , #52 , #63 , #97 , #99 , #100, #101, #103, #124

This is a screenshot from out private thread, where the sponsor @maxweng explains the function functionality. Maybe he can confirm here in the comments for more clarity.

Screenshot from 2024-07-21 17-08-05

Please note that maxClaimable[token] is always 0 by default. As the function usage is a rare case and maxClaimable[token] is purely intended to be referenced in this function, I believe it's a Low-severity one at max. (Edit: again, in my opinion, this is exactly the same vulnerability as https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/33 is)

Just to clarify further, there need to be 3 pre-conditions for this attack to be exploitable:

1) The admin needs to set maxClaimable[token] to a non-zero value, 2) the VouchFacet contract will need to hold funds (e.g. leftover after an exit); 3) These funds should remain in the contract (at least for a single block, though 1 block is sufficient for that), and the attack can happen then.

cholakovvv commented 1 month ago

maxClaimable[token] is set by the owner in setMaxClaimable whenever he wants, the function usage being a rare case, doesn't mean that the owner will not call setMaxClaimable and set the maxClaimable.

Yes the contract will need to hold funds.

And it should be at least Medium severity, as the issue can lead to a big loss of funds.

cholakovvv commented 1 month ago

I believe it would be best some sponsor to clarify the intended functionality of the function, including when setMaxClaimable and other related actions should occur. He should make the final decision on all issues related to this function.

mystery0x commented 1 month ago

This is a different root cause associated with claimTokens() as described in #33.

c-plus-plus-equals-c-plus-one commented 1 month ago

This is a different root cause associated with claimTokens() as described in #33.

@mystery0x

Well, it is NOT a different root cause.

If you think it is different, please explain why do you think so in spite of clear evidence:

The "stated" "culrpit" is the claimTokens function.

Additionally, it was clarified that the function will be very rarely calleable based on the sponsor's comment (see screesnhots above).

It's absolutely the same issue, in my opinion. 🤔

0xAadi commented 1 month ago

The "stated" "culrpit" is the claimTokens function.

IMO, there are three issues in this function:

  1. There is no whitelisting, proper access control or restriction preventing unauthorized users from executing this function.
  2. The claimedTokens[token][msg.sender] is not being updated while executing this function.
  3. This issue occurs only in the absence of issue 2. The current claim amount is not considered in this check: require(claimedTokens[token][msg.sender] <= maxClaimable[token], "amount>max");.

Issues 2 and 3 are grouped under #33 because both issues can be resolved with a single fix, which is updating claimedTokens[token][msg.sender] before this check: require(claimedTokens[token][msg.sender] <= maxClaimable[token], "amount>max");.

Issue 1 requires a separate fix. I think that’s why it’s grouped separately.

It was clarified that the function will be very rarely callable based on the sponsor’s comment (see screenshots above).

There is no concrete evidence stating that it’s rarely callable or meant only for rescuing tokens. There is no evidence either in the README or in the CODE COMMENTS. In this situation, the only possible source of truth is the code itself.

If this function is intended only for the admin, then there is no need for maximum claimable restrictions, and the amount will be directly transferred to the owner() address.

Usually, sponsor’s private replies are not considered as a valid source of truth in Sherlock. Please Refer

III. Sherlock's standards:

  1. Discord messages or DM screenshots are not considered sources of truth while judging an issue/escalation especially if they are conflicting with the contest README.
c-plus-plus-equals-c-plus-one commented 1 month ago

The "stated" "culrpit" is the claimTokens function.

IMO, there are three issues in this function:

  1. There is no whitelisting, proper access control or restriction preventing unauthorized users from executing this function.
  2. The claimedTokens[token][msg.sender] is not being updated while executing this function.
  3. This issue occurs only in the absence of issue 2. The current claim amount is not considered in this check: require(claimedTokens[token][msg.sender] <= maxClaimable[token], "amount>max");.

Issues 2 and 3 are grouped under #33 because both issues can be resolved with a single fix, which is updating claimedTokens[token][msg.sender] before this check: require(claimedTokens[token][msg.sender] <= maxClaimable[token], "amount>max");.

Issue 1 requires a separate fix. I think that’s why it’s grouped separately.

It was clarified that the function will be very rarely callable based on the sponsor’s comment (see screenshots above).

There is no concrete evidence stating that it’s rarely callable or meant only for rescuing tokens. There is no evidence either in the README or in the CODE COMMENTS. In this situation, the only possible source of truth is the code itself.

If this function is intended only for the admin, then there is no need for maximum claimable restrictions, and the amount will be directly transferred to the owner() address.

Usually, sponsor’s private replies are not considered as a valid source of truth in Sherlock. Please Refer

III. Sherlock's standards:

  1. Discord messages or DM screenshots are not considered sources of truth while judging an issue/escalation especially if they are conflicting with the contest README.

@0xAadi, the sponsor has marked the issue #33 with a Won't fix label, which means that the ability to withdraw funds chaotically when setMaxClaimable has been used to increase the maxClaimable[token] to a non-0 value.

The fix suggested by #33 is anyways absolutely incorrect.

I DO believe that (according to Sherlock's judging rules which state that the main culprit should be used as a determination for issue's duplicates' existence and proper grouping, not the suggested fix, as the suggested fix is not a mantadory field AT ALL) we should group issues as duplicates based on what is the exploitable attack vector, and what is the the endangered part of the code.

  1. Your statement â„–1 is wrong:

    The `claimTokens` function *is* protected as `maxClaimable[token]` is always `0` by default, and it can only be set specifically if an admin wishes so. Admin is a retricted entity, so he knows what he is doing.

    There're no `setUserClaimableBalance` functions in the `VouchFacet` contract at all, which means that **enabling all users to claim is the current intended behavior of the flow consisting of setting `setMaxClaimable` to a non-zero value.

    I believe that the `setMaxClaimable` is only called in the scenario when the admin WANTS to give the other users rights to withdraw for some reason, such as when the use of this transaction is to allow other contracts execute **in-motion** transfers, **as in a multisig's batch call for example**, and then normally claimable will be reset to 0 at the end of the batch call.

    Note that in order for the `claimTokens` function to transfer at least SOMETHING, the `VouchFacet` should hold funds, which wouldn't normally be the case

  2. Considering all of the 3 preconditions that I have provided in one of my comments above:

    > ### Just to clarify further, there need to be 3 pre-conditions for this attack to be exploitable: > 1) The admin needs to set `maxClaimable[token]` to a non-zero value, > 2) the `VouchFacet` contract will need to hold funds (e.g. leftover after an `exit`); > 3) These funds should remain in the contract (at least for a single block) ... I believe that, if not invalid, this issue is a fair "Low-severity" at max.

If the sponsor decided not to fix #33, then doesn't it logically mean that this behavior is intended?

c-plus-plus-equals-c-plus-one commented 1 month ago

@0xAadi

Additionally, there would be absolutely NO SENSE in adding the onlyOwner modifier to the claimTokens function, as the

    /// @notice Transfer ERC20 tokens
    function transferERC20(address token, address to, uint256 amount) external onlyOwner {
        IERC20(token).transfer(to, amount);
    }

(transferERC20 function quoted above) is ALREADY sufficient at transferring tokens to the owner-specified address.

I think it's correct that we should group issues as duplicates based on *what is the exploitable attack vector that they have in common, and what is the the endangered part of the code that they're informing the protocol about.

From Sherlock's rules:

image

Notice that BOTH the conceptual culprit and the linked code sources are the same for #96 and #33.

WangSecurity commented 1 month ago

Firstly, labels "sponsor confirmed/disputed" and "will/won't fix" do not affect the judging, so don't use it as an argument to validate/invalidate any of the reports.

Secondly, this function only retrieves the funds transferred in the contract by a user mistake. Hence, if the user mistakenly transferred tokens in the contract directly and they're used by another user, it's low severity, since it requires a mistake from the user.

Thus, I believe that this report should be invalid (low). But, since the escalations ask to look at a set of different reports, I'll give my decision about each report mentioned later.

0xAadi commented 1 month ago

@WangSecurity

this function only retrieves the funds transferred in the contract by a user mistake.

There are other ways the contract can receive funds other than funds transferred by users mistakenly

If I'm not wrong, there are three ways this contract can receive funds:

  1. Users mistakenly transfer funds.
  2. Through exit(): If the owner calls exit(), the accrued rewards and the unstaked amount will be transferred to this contract.
  3. Through stake(): If the stake() function is executed, the accrued rewards will be transferred to this contract. Please see the following execution path for more clarity.
    1. VouchFaucet.stake()
    2. UserManager.stake()
    3. Comptroller.withdrawRewards()

Since there is no access control in stake(), an attacker can call this function with a negligible amount of staking tokens and use the claimToken() function to steal the reward from the protocol. Similarly, an attacker can backrun exit() to steal the unstaked amount as well as rewards.

c-plus-plus-equals-c-plus-one commented 1 month ago

@WangSecurity

this function only retrieves the funds transferred in the contract by a user mistake.

There are other ways the contract can receive funds other than funds transferred by users mistakenly

If I'm not wrong, there are three ways this contract can receive funds:

  1. Users mistakenly transfer funds.
  2. Through exit(): If the owner calls exit(), the accrued rewards and the unstaked amount will be transferred to this contract.

  3. Through stake(): If the stake() function is executed, the accrued rewards will be transferred to this contract. Please see the following execution path for more clarity.

    1. VouchFaucet.stake()
    2. UserManager.stake()
    3. Comptroller.withdrawRewards()

Since there is no access control in stake(), an attacker can call this function with a negligible amount of staking tokens and use the claimToken() function to steal the reward from the protocol. Similarly, an attacker can backrun exit() to steal the unstaked amount as well as rewards.

Based on my understanding, normally the setMaxClaimable function will be only used to enable non-staked tokens to be rescued through the claimTokens permissionless function. Note that the setMaxClaimable function is restricted to only the VouchFacet's admin role.

Thus, maxClaimable will always be 0 for the assets that the VouchFacet's owner really stakes.

I also believe the following rule applies:

Admin functions are assumed to be used properly, unless a list of requirements is listed and it's incomplete or if there is no scenario where a permissioned funtion can be used properly.

image

And in my opinion, the only case when an admin would use the setMaxClaimable function is when he wants to enable rescuing 3rd-party tokens.

Because the restricted transferERC20 function already handles transferring any tokens with no restrictions.

cholakovvv commented 1 month ago

Based on my understanding, normally the setMaxClaimable function will be only used to enable non-staked tokens to be rescued through the claimTokens permissionless function. Note that the setMaxClaimable function is restricted to only the VouchFacet's admin role. Thus, maxClaimable will always be 0 for the assets that the VouchFacet's owner really stakes.

This is not true; these are only superstitions, as there is no place that states this. The admin can call it whenever he wants to set the max claimable amount. So, this does not mean that maxClaimable will always be 0.

WangSecurity commented 1 month ago

As I understand, this function is indeed should be called by the users directly, not by the admin. Even though as we can see this function is for rescuing tokens (from the screenshot where the sponsor says it) still it doesn’t mean it should be restricted to the admin. Secondly, the admin has their function to transfer tokens, which can be used to rescue tokens as well (transferERC20). Therefore, I believe this function indeed shouldn’t have the onlyOwner modifier and should be accessible by the users.

Therefore, I agree with @neko-nyaa escalation. It will be accepted, this issue and its duplicate #107 will be invalidated. As I see, the other two are indeed duplicates of #33.

About the second escalation, I believe #33 correctly points out the problem in the require statement. But, as we see, this function to claim tokens is used only to rescue tokens if they’re sent in the contract directly, which is a user mistake. Still there’s an issue that doesn’t check correctly how many tokens have already been claimed by the user and allows anyone to get them besides the user who sent the tokens. While this issue involves user mistake, there’s still a contract functionality to get them. Hence, I believe medium is more appropriate. Planning to reject @cholakovvv escalation, since it states the issue should be invalid, while I believe it should med, but will downgrade the severity to Medium.

cholakovvv commented 1 month ago

@WangSecurity I believe this issue should be valid, as the function to be callable only by the admin is just a suggestion, both this and #33 share the same root cause, which is that the contract can be drained by any user and this issue also states the lack of proper validation:

  1. There is no validation of the amount parameter specified by the use, allowing him so withdraw all the tokens in the contract.

Please reconsider it. Thanks!

0xAadi commented 1 month ago

@WangSecurity

I believe #33 should be classified as high severity.

this function to claim tokens is used only to rescue tokens if they’re sent in the contract directly, which is a user mistake.

This is not entirely accurate. The vulnerability discussed in #33 still allows attackers to steal unstaked tokens and rewards from the voucher contract.

Please see this comment

0xKorok commented 1 month ago

@WangSecurity

I agree with @0xAadi that the original judgement is correct and that #33 should maintain its high classification.

His linked comment accurately highlights the multiple logical flows that result in VouchFaucet legitimately receiving funds (rewards and unstaked amounts) that can in no way be said to be a user mistake.

I think it’s important to set aside the theoretical intended design of claimTokens function and focus on what a malicious actor could do with it if deployed as-is. Issue #33 demonstrates that in the current state VouchFaucet is completely insecure (any token received is trivially drainable by anyone).

I believe the combination of these insights make a strong case for maintaining high classification.

Additional details:

c-plus-plus-equals-c-plus-one commented 1 month ago

@WangSecurity The issue requires a prerequisite of privileged admin actions (calling setMaxClaimable is only restricted to the admin role) prior to becoming a problem, and without admin calling this function, no one will be able to call the claimTokens function. So this is not an issue per se as long as the admin knows what he is doing. Also, the issue is not a problem if there're no funds leftover in the VouchFacet contract.

Funds are supposed to be transferred to the UserManager in-motion, and shold not remain in the contract passively.

0xKorok commented 1 month ago

@c-plus-plus-equals-c-plus-one

Can you explain why you believe the admin needs to call setMaxClaimable? From what I can tell the issues is always present for all tokens.

This is because the logical error in the claimTokens require statement ensures that it passes in all possible cases. Having the admin call or not call setMaxClaimable doesn’t change this in anyway.

To see this for yourself I encourage you to run this modified version of the foundry PoC I included in #33 (expandable code section below). It uses the protocols own TestWrapper and is very simple to setup. The test passes without any prerequisite privileged actions being taken.

Foundry Proof of Concept ```solidity // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.16; import {Test, console} from "forge-std/Test.sol"; import {TestWrapper} from "../TestWrapper.sol"; import {VouchFaucet} from "../../src/contracts/peripheral/VouchFaucet.sol"; import {IERC20} from "@openzeppelin/token/ERC20/IERC20.sol"; contract TestVouchFaucet is TestWrapper { VouchFaucet public vouchFaucet; uint256 public TRUST_AMOUNT = 10 * UNIT; function setUp() public { deployMocks(); vouchFaucet = new VouchFaucet(address(userManagerMock), TRUST_AMOUNT); } function testDrainVouchFaucet() public { address bob = address(1234); erc20Mock.mint(address(vouchFaucet), 100 * UNIT); vm.prank(bob); vouchFaucet.claimTokens(address(erc20Mock), 100 * UNIT); assertEq(IERC20(erc20Mock).balanceOf(bob), 100 * UNIT); } } 


<img width="1320" alt="drainTest" src="https://github.com/user-attachments/assets/f07b88b6-11c7-4f17-9d2b-691fa9efc0d1">
c-plus-plus-equals-c-plus-one commented 1 month ago

@c-plus-plus-equals-c-plus-one

Can you explain why you believe the admin needs to call setMaxClaimable? From what I can tell the issues is always present for all tokens.

This is because the logical error in the claimTokens require statement ensures that it passes in all possible cases. Having the admin call or not call setMaxClaimable doesn’t change this in anyway.

To see this for yourself I encourage you to run this modified version of the foundry PoC I included in #33 (expandable code section below). It uses the protocols own TestWrapper and is very simple to setup. The test passes without any prerequisite privileged actions being taken.

Foundry Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.16;

import {Test, console} from "forge-std/Test.sol";
import {TestWrapper} from "../TestWrapper.sol";
import {VouchFaucet} from "../../src/contracts/peripheral/VouchFaucet.sol";

import {IERC20} from "@openzeppelin/token/ERC20/IERC20.sol";

contract TestVouchFaucet is TestWrapper {
    VouchFaucet public vouchFaucet;
    uint256 public TRUST_AMOUNT = 10 * UNIT;

    function setUp() public {
        deployMocks();
        vouchFaucet = new VouchFaucet(address(userManagerMock), TRUST_AMOUNT);
    }

    function testDrainVouchFaucet() public {
        address bob = address(1234);
        erc20Mock.mint(address(vouchFaucet), 100 * UNIT);

        vm.prank(bob);
        vouchFaucet.claimTokens(address(erc20Mock), 100 * UNIT);

        assertEq(IERC20(erc20Mock).balanceOf(bob), 100 * UNIT);
    }

}
</details>
drainTest

Aaah, I see now, apologies.

Then the only pre-requisiste is that the VouchFacet has tokens, and that token is not USDT.

Because according to #22 non-safe transfer on USDT will always fail due to an interface mismatch.

WangSecurity commented 1 month ago

I believe this issue should be valid, as the function to be callable only by the admin is just a suggestion, both this and https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/33 share the same root cause, which is that the contract can be drained by any user and this issue also states the lack of proper validation

@cholakovvv Yep, excuse me, overlooked this part. Indeed I agree that this report identifies there's an incorrect require statement, even though it's not clearly expressed, I believe it's sufficient. Same for #107, it's not very clear, but indeed it identifies that the function doesn't have sufficient validation for the user that can claim tokens.

This is not entirely accurate. The vulnerability discussed in https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/33 still allows attackers to steal unstaked tokens and rewards from the voucher contract

@0xAadi I've seen your previous comment, still this function will only be used to rescue tokens. And there's a maxClaimable variable that prevents all tokens from being withdrawn. Therefore, I believe the correct way this function should be used is if someone sends the tokens directly into the contract and the admin increases maxClaimable so the user can get them. Otherwise, when the owner calls exit for example, they can use the transferERC20 function to get the tokens. Hence, this function is only for rescuing tokens as evidenced by the sponsor's explanation. The same applies to @0xKorok comment. I understand that the tokens can get into a contract in different ways, but still, there's a maxClaimable limitation that prevents from the attackers getting any funds that get into a contract if used properly.

The issue requires a prerequisite of privileged admin actions (calling setMaxClaimable is only restricted to the admin role) prior to becoming a problem, and without admin calling this function, no one will be able to call the claimTokens function. So this is not an issue per se as long as the admin knows what he is doing. Also, the issue is not a problem if there're no funds leftover in the VouchFacet contract.

@c-plus-plus-equals-c-plus-one I understand, but this function is solely to rescue tokens that were directly transferred into the contract and for this function to ever work, the admin has to set maxClaimable to any value >0. Therefore, I believe it's completely a viable scenario that an admin can set maxClaimable to any value >0 to make the function work. And I see you also sorted out this question with @0xKorok.

Hope that answers your questions. The decision remains as it is. Planning to reject the escalation and leave the issue as it is.

0xKorok commented 1 month ago

@WangSecurity Thanks for continuing to look at this I know you have a lot of your plate.

Hence, this function is only for rescuing tokens as evidenced by the sponsor's explanation. The same applies to @0xKorok comment. I understand that the tokens can get into a contract in different ways, but still, there's a maxClaimable limitation that prevents from the attackers getting any funds that get into a contract if used properly.

I don't believe this statement is accurate. Currently the maxClaimable variable doesn’t prevent anything. An attacker can withdraw all contract funds at any moment no matter now "properly" the contract is used.

I think the sponsor was well-meaning when they shared, and maybe continue to insist, that the intended design is "only for rescuing tokens". I think this particular statement from the sponsor has and continues to cause much confusion for watsons and for the judging process around this issue.

The reality is that this "only for rescuing tokens" idea is a fantasy. In it's current form the claimTokens function is equivalent to and unrestricted version of the admin protected transferERC20 function, and the VouchFaucet contract is completely useless for its intended purpose.

Honestly it's getting a bit hard to reason about the current state of this and related issues. My current question is:

Do you consider my report (33) to be a duplicate of this issue (96)? I personally think the original judge did the right thing separating the two reports, but I recognize that this is highly subjective. I think they are not duplicates and there is a strong case for invalidating this particular report (96). Mainly because their presentation of the issue got so many important aspects wrong that I feel it's inaccurate to call it a satisfactory report. For example:

Curious on your thoughts. Personally I think #33 is a provably correct, high severity, and the most accurate representation of the issue. While others may have been close, and we should examine them individually, I think their reports should be invalid if they presented critical aspects of the issue like root cause, attack path, and mitigation incorrectly or didn't describe the vulnerability in comparable detail to 33.

WangSecurity commented 1 month ago

Yep, I see where my misunderstanding comes from. Indeed, any second the tokens get into the contract, they can be immediately claimed by anyone with every value of maxClaimable set by an admin.

In that sense, I agree it’s high severity.

About this issue being duplicates of #33. I agree they don’t indicate the root cause very clear, still I believe they do. For example, this issue still identifies that the amount parameter is not validated in the require statement. I see that the pre-condition listed in this report is not needed. Still, it doesn’t change the fact that the root cause of not checking the amount in the require statement is in the report. About the attack path, they used the scenario about Alice sending tokens directly in the contract because the purpose of this function. I see this function can be used for stealing all the tokens inside the contract, but it doesn’t change the fact that its initial purpose is to rescue tokens. Therefore, I agree it’s a user mistake, but there’s a functionality to get these tokens and it’s broken. Hence, I believe it’s sufficient for medium severity. About mitigation, correctness of the mitigation doesn’t affect the validity or the duplication status of the report.

Hope that answers your questions. The decision remains the same, accept @neko-nyaa escalation, reject @cholakovvv escalation and duplicate this report with #33 and keep #33 as best.

0xKorok commented 1 month ago

@WangSecurity

I’m new to Sherlock escalations, just for my understanding:

  1. On Sherlock is it possible that reports that are considered duplicates receive different severity classifications based on report quality? Is that what will happen in this case?

I ask because the neko-nyaa escalation you plan to accept doesn’t dispute that 33 is high severity, and it seems like you now agree. However you also agree that 96 is a duplicate of 33 and plan to override the accepted escalations recommendation around 96 (invalidate) with medium. Am I understanding the current state correctly?

WangSecurity commented 1 month ago

Answering the question, all the duplicates inside one issue family have the same severity, i.e. all are high or all are medium. The severity depends on the highest severity found, i.e. if one report found only medium, but the second found high impact, then both are assigned High severity.

Why I plan to accept @neko-nyaa escalation is because it identified that the issue family was incorrectly judged, which I agree with. Yes, it claims that this issue should be invalid. I disagree with that part, but overall, still, the escalation is correct identifying the judging mistake. Does it answer your questions? If not, tell me what’s missing, I’ll try to elaborate more.

The decision remains the same, accept @neko-nyaa escalation, reject @cholakovvv escalation, duplicate this report and its duplicates with #33.

WangSecurity commented 1 month ago

Answering the question, all the duplicates within one family have the same severity, i.e. either all are high or all are medium. The chosen severity is defined based on the highest severity found, i.e. if 9 duplicates found only medium and 1 duplicate found high, then all are assigned High severity.

Why I plan to accept @neko-nyaa escalation is because it identifies that this family is incorrectly judged and indeed the issue shouldn’t be separated from #33. I disagree with the escalation that this report should be invalid, but still the escalation correctly identified the family is judged incorrectly.

Hope that answers your question, if not, let me know what’s missing and I’ll try to elaborate more.

The decision remains the same, accept @neko-nyaa escalation, reject @cholakovvv escalation, duplicate this issue family with #33.

0xKorok commented 1 month ago

@WangSecurity

Is there any monetary incentive to encourage watsons to try to submit the issue that is selected for inclusion into the report within an issue family?

WangSecurity commented 1 month ago

Unfortunately, there is no monetary incentive for it now, but it may appear in the future, so you can train writing the best reports for now.

0xKorok commented 1 month ago

@WangSecurity

Your answer helps me understand why most reports on this issue are written as they are. It seems watsons are given pretty significant wiggle room regarding the quality of the report, they just have to be ready to fight it out when necessary in the escalations.

Another question for my understanding:

  1. Certain watsons submitted issues they believed to be distinct that are now all grouped under the same issue family as #33, for example issues #107, #51, #52. In these cases it seems like these issues will be combined into a single issue for rewards purposes and carry the same weight as 33 within the issue family, is my understanding correct?
WangSecurity commented 1 month ago

Your answer helps me understand why most reports on this issue are written as they are. It seems watsons are given pretty significant wiggle room regarding the quality of the report, they just have to be ready to fight it out when necessary in the escalations

Yep, that's what we're trying to fight and not reward poor quality reports, but there's still for improvement in our system.

these cases it seems like these issues will be combined into a single issue for rewards purposes and carry the same weight as 33 within the issue family, is my understanding correct

Hm, yep, the Lead Judge has grouped all the issues inside claimTokens in one issue family. tagging @mystery0x and will look into these issues, as I see there are other Watsons who submitted issues different issues in different report, like in the ones you've sent.

WangSecurity commented 1 month ago

I see there are three families:

  1. Require statement checks only for the amount already claimed, not considering what will be claimed now.
  2. claimedTokens is never set and always defaults to 0.
  3. Require statement checks for the user individual claimedToken and for global maxClaimable for each token, when maxClaimable should also be individual for each user.

I believe the last is invalid and is a design recommendation and maxClaimable is intended to be a global variable, but the first 2 I believe should be duplicated, since they both find a root cause of not updating the claimedTokens mapping appropriately.

Hence, the valid issues with high severity are:

This report is invalid. It identifies 3 issues, the first is invalid as was said in a couple of previous comments, and the third is invalid based on the above. The second issue:

There is no validation of the amount parameter specified by the use, allowing him so withdraw all the tokens in the contract.

I believe it's too vague and doesn't clearly find a root cause of the claimedTokens not being updated with the specified amount value or verifying that claimedTokens + amount doesn't exceed maxClaimable.

107 -- identifies that maxClaimable should be assigned to each specific address. Invalid based on the above.

100 -- factually incorrect saying that claimedTokens is initialised before the claim but amount parameter can exceed the claimedTokens while claimedTokens is how many tokens the user has already claimed, not how many tokens it can claim

63 -- too vague and while it states the claimedTokens is not updated, I don't see it qualifying to be a duplicate due to insufficient attack/vulnerability path (the one sentence in the report I believe is not sufficient).

Hence, planning to accept @neko-nyaa escalation, reject @cholakovvv escalation and apply the changes expressed above. Hope that answers all the questions. If I missed your report, let me know.

0xKorok commented 1 month ago

@WangSecurity

I see that among your selected valid issues 3 watsons have 2 distinct reports submitted, for example 51 and 52.

  1. Will two valid reports in this issue family receive twice the share of this issues reward allocation, or will these watsons receive a single share of the allocation for reward purposes?
WangSecurity commented 1 month ago

@0xKorok yep, excuse me for not answering earlier. They will receive the payout and points for only one, i.e. these watsons will receive a single share of the allocation for reward. The second report won’t be rewarded at all.

WangSecurity commented 4 weeks ago

Result: Invalid Has duplicates

sherlock-admin4 commented 4 weeks ago

Escalations have been resolved successfully!

Escalation status: