Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uninitialized Variable marketWhitelist in RCTreasury.sol #18

Open
code423n4 opened this issue Aug 22, 2021 · 3 comments
Open

Uninitialized Variable marketWhitelist in RCTreasury.sol #18

code423n4 opened this issue Aug 22, 2021 · 3 comments
Labels
2 (Med Risk) bug Something isn't working disagree with severity Resolved Used when a fix has been implemented. sponsor confirmed

Comments

@code423n4
Copy link
Contributor

Handle

leastwood

Vulnerability details

Impact

The variable, marketWhitelist, is never initialized in the contract RCTreasury.sol. As a result, the function marketWhitelistCheck() does not perform a proper check on whitelisted users for a restricted market. Additionally, the function will always return true, even if a market wishes to restrict its users to a specific role.

Proof of Concept

The initial state variable is defined in the link below.
https://github.com/code-423n4/2021-08-realitycards/blob/main/contracts/RCTreasury.sol#L75

The state variable marketWhitelist is accessed in the function RCTreasury.marketWhitelistCheck() as per below.
https://github.com/code-423n4/2021-08-realitycards/blob/main/contracts/RCTreasury.sol#L269-L281

The function RCTreasury.marketWhitelistCheck() is called in RCMarket.newRental() as seen below. The comment indicates that there should be some ability to restrict certain markets to specific whitelists, however, there are no methods in RCTreasury that allow a market creator to enable this functionality.
https://github.com/code-423n4/2021-08-realitycards/blob/main/contracts/RCMarket.sol#L758-L761

Tools Used

npx hardhat coverage
slither
Manual code review

Recommended Mitigation Steps

Ensure this behaviour is intended. If this is not the case, consider adding a function that enables a market creator to restrict their market to a specific role by whitelisting users.

@Splidge
Copy link
Collaborator

Splidge commented Aug 25, 2021

I think the severity could be double-checked on this one.
It's a close one but I'd be tempted to put it under 1 (low risk) as a "Function incorrect to spec".
Regardless, this will be fixed.

Edit: I notice the duplicates were both marked as 1 (low risk).

@0xean
Copy link
Collaborator

0xean commented Sep 2, 2021

based on " but the function of the protocol or its availability could be impacted" in the code4 docs, I am going to agree with warden and leave this as a 2. The function of the protocol is certainly impacted in a case where the whitelist if not working correctly.

@Splidge
Copy link
Collaborator

Splidge commented Sep 7, 2021

Fixed here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) bug Something isn't working disagree with severity Resolved Used when a fix has been implemented. sponsor confirmed
Projects
None yet
Development

No branches or pull requests

3 participants