trachev - Any user can claim an unlimited amount of vouch in VouchFaucet.sol

[sherlock-admin4]

trachev

High

Any user can claim an unlimited amount of vouch in VouchFaucet.sol

Summary

Currently, there is no validation performed in VouchFaucet.sol when claimVouch is called. This is highly dangerous as any untrusted user can increase their vouch and perform malicious borrows, stealing from the contract's stake.

Vulnerability Detail

As we can see in the claimVouch function there is no validation performed and it can be called by any address:

function claimVouch() external {
        IUserManager(USER_MANAGER).updateTrust(msg.sender, uint96(TRUST_AMOUNT));
        emit VouchClaimed(msg.sender);
}

In addition to that, the maximum amount of trust that any user can claim - TRUST_AMOUNT can easily be bypassed by calling claimVouch, borrowing the entire TRUST_AMOUNT and after that calling claimVouch again.

Impact

Malicious borrows can be made by untrusted users and the maximum amount that can be vouched for a user can be bypassed, putting the contract's funds at risk of being stolen.

Code Snippet

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/peripheral/VouchFaucet.sol#L87-L90

Tool used

Manual Review

Recommendation

Only users approved by the owner should be able to call claimVouch and they should not be able to claim more trust than TRUST_AMOUNT.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Intended design. (borrower == staker) is checked to prevent self vaouching in updateTrust()

[trachevgeorgi]

Escalate
This issue has been incorrectly excluded. The comment from @mystery0x is not connected to the issue as the problem is not related to whether the borrower is the same as the staker.
The issue revolves around the fact that any user can claim more trust than the allowed limit (TRUST_AMOUNT), thus allowing any user to claim as much trust as they wish from the VouchFaucet.sol contract. As a result, a single user can block any other users from successfully calling the claimVouch function.
This is definitely not the intended behaviour of the function as most likely only one user will claim all of the contract's trust.

[sherlock-admin3]

Escalate
This issue has been incorrectly excluded. The comment from @mystery0x is not connected to the issue as the problem is not related to whether the borrower is the same as the staker.
The issue revolves around the fact that any user can claim more trust than the allowed limit (TRUST_AMOUNT), thus allowing any user to claim as much trust as they wish from the VouchFaucet.sol contract. As a result, a single user can block any other users from successfully calling the claimVouch function.
This is definitely not the intended behaviour of the function as most likely only one user will claim all of the contract's trust.

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.

[mystery0x]

There is onlyMember visibility in updateTrust():

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/user/UserManager.sol#L586

    function updateTrust(address borrower, uint96 trustAmount) external onlyMember(msg.sender) whenNotPaused {

It's a permissioned call eventually, i.e. only member can vouch someone. Given the context of the design, it doesn't seem to introduce a threat unless VouchFaucet.sol is a member.

[trachevgeorgi]

@mystery0x With all due respect, I believe you do not understand the issue correctly. The VouchFaucet.sol contract is going to be staking in Union, like a member. Therefore, after it stakes borrowers are going to be able to borrow assets from the VoucheFaucet.sol contract, like they would from any other member. The only way to borrow from a member is if the lender (VouchFaucet.sol in the case) calls updateTrust with the specific borrower as the first parameter. Essentially, the borrower is vouching for / increasing their trust in a certain member.
In the issue above any address can increase their own trust on behalf of VouchFaucet.sol. The issue is that they can give themselves too much trust, due to insufficient validation. As a result, they are going to be able to borrower the entire stake of VouchFaucet.sol instead of the allowed TRUST_AMOUNT.

[WangSecurity]

As I understand not every staker is necessarily a member, so @trachevgeorgi could you elaborate more on how VouchFacet can become a member for updateTrust to be called?

[trachevgeorgi]

@WangSecurity Yes, I can. There are two ways that any address can become a member. The first one is the addMember onlyAdmin function, which allows the admin to make any address a member for free. The second one is the registerMember(address newMember) function, which allows users to make other addresses members by paying a fee of Union Token: https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/user/UserManager.sol#L722-L728
The owner of the VouchFoucet.sol contract will most likely use the second option.

Furthermore, it is important to add that if the VouchFaucet.sol contract is not a member, it is not going to be able to function, so being a member is an expected necessity.

[WangSecurity]

Furthermore, it is important to add that if the VouchFaucet.sol contract is not a member, it is not going to be able to function, so being a member is an expected necessity.

Can you elaborate more about it and where you got this info from?

Sorry for the late reply, I'm trying to get info on VouchFacet from the sponsor.

[trachevgeorgi]

Furthermore, it is important to add that if the VouchFaucet.sol contract is not a member, it is not going to be able to function, so being a member is an expected necessity.

Can you elaborate more about it and where you got this info from?

Sorry for the late reply, I'm trying to get info on VouchFacet from the sponsor.

Hi, I believe that many users will be members as this is the only way to actually lend your money. Being a member cannot be a very restrictive role that only certain addresses trusted by the protocol can have access to. We understand that as any address can register themselves or others as members by just paying a fee of Union Token, so it is definitely intended for any address to be possible and not difficult to become a member.
Regarding your question on where I get the information from, if you look at the registerMember function and the developer comments above it you can get all of the information necessary: https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/7ffe43f68a1b8e8de1dfd9de5a4d89c90fd6f710/union-v2-contracts/contracts/user/UserManager.sol#L715-L728

Also it is important to notice that in order to vouch for somebody (vouching essentially means that you are allowing a borrower to borrow from your funds) you need to be a member, therefore all lenders will need to be members.

The purpose of VouchFaucet is for it to stake in the protocol and after that for the contract to be able to vouch for other users so that the users will be able to borrow from the VouchFaucet's stake, for this to work it is expected for VouchFaucet to be a member.

[trachevgeorgi]

@WangSecurity To add to the comment above, my report essentially indicates the issue that any user can increase how much the VouchFaucet.sol contract, which must be a member, has vouched for them, even though they should only be allowed to get only TRUST_AMOUNT of trust out of VouchFaucet.sol. For example, if VouchFaucet.sol has a stake of 2 * TRUST_AMOUNT, a single user should only be able to get 1 * TRUST_AMOUNT, and therefore be able to borrow only 1 * TRUST_AMOUNT, but currently they can borrow the entire 2 * TRUST_AMOUNT.

[WangSecurity]

Thank you, I understand why it's important for the users to be members, but I meant why it's important for VouchFacet contract to be a member, specifically. Excuse me for the confusion, is this information in the docs or you cam to this conclusion by just getting more context of the code base?

I see VouchFacet is also used to stake in the User Manager. And agree it would be strange that there's a function to give trust but this contract wouldn't be a member.

What makes me confused here is that on one of other escalations one Watson shared a screenshot from the sponsor saying VouchFacet::claimTokens is just to rescue tokens that were accidentally sent into the contract. So that makes me confused about the entire VouchFacet and why it's going to be used.

[trachevgeorgi]

Thank you, I understand why it's important for the users to be members, but I meant why it's important for VouchFacet contract to be a member, specifically. Excuse me for the confusion, is this information in the docs or you cam to this conclusion by just getting more context of the code base?

I see VouchFacet is also used to stake in the User Manager. And agree it would be strange that there's a function to give trust but this contract wouldn't be a member.

What makes me confused here is that on one of other escalations one Watson shared a screenshot from the sponsor saying VouchFacet::claimTokens is just to rescue tokens that were accidentally sent into the contract. So that makes me confused about the entire VouchFacet and why it's going to be used.

@WangSecurity Hi! There are no documentations on this contract but I believe it is pretty clear what its purpose is just by looking at the code and the contract’s name. Firstly, the name VouchFaucet indicates that it is another way for users to vouch for other users. They just stake their funds and allow others to claim a certain amount of trust in order to borrow from the stake. It is impossible for claimVouch to be used to rescue anything as its sole purpose is to allow addresses to claim an amount of trust. I have pointed two issues with the code the main one being able to bypass the TRUST_AMOUNT limit which is why I believe the finding should be valid.

[maxweng]

Hi, this contract is only used for testing purposes on Testnet. And claimVouch() is just to allow testers to easily get vouches even no other members do it for them.

[WangSecurity]

But as I understand this wasn't mentioned anywhere during the contest, correct?

Also, @trachevgeorgi as I understand, even if VouchFaucet is not a member, any existing member can give vouchers to VouchFaucet and then call registerMemeber to make VouchFaucet a member, correct? In that way, this bug would be possible, if we disregard the fact that it's not intended to be deployed. Also, is there a way to remove a member?

[trachevgeorgi]

But as I understand this wasn't mentioned anywhere during the contest, correct?

Also, @trachevgeorgi as I understand, even if VouchFaucet is not a member, any existing member can give vouchers to VouchFaucet and then call registerMemeber to make VouchFaucet a member, correct? In that way, this bug would be possible, if we disregard the fact that it's not intended to be deployed. Also, is there a way to remove a member?

@WangSecurity Yes, anyone can make VouchFaucet a member and just as you said it was never mentioned where the contract will be deployed therefore it is still in scope. Also I do not find a function that can remove an address from the member list.

[WangSecurity]

Then, I agree it has to be a valid finding. But, the medium is more appropriate because it doesn't necessarily lead to a loss of funds, the malicious user cannot increase the trust to infinity. It will always be equal to TRUST_AMOUNT which can be small, there's a maximum number of vouchers that one member can have.

Hence, planning to accept the escalation and validate with medium severity.

[trachevgeorgi]

Then, I agree it has to be a valid finding. But, the medium is more appropriate because it doesn't necessarily lead to a loss of funds, the malicious user cannot increase the trust to infinity. It will always be equal to TRUST_AMOUNT which can be small, there's a maximum number of vouchers that one member can have.

Hence, planning to accept the escalation and validate with medium severity.

I am ok with a medium evaluation. I will not be escalating further.

[WangSecurity]

Result:
Medium
Unique

@mystery0x @trachevgeorgi are there any duplicates?

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• trachevgeorgi: accepted