search

code-423n4 / 2021-06-realitycards-findings

3 stars 2 forks source link

Deposit whitelist enforced on msg.sender instead of user #107

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The Treasury deposit() function credits amount to the user address in parameter instead of the msgSender() that is actually making the deposit whose rationale as explained in the Natspec comment is that this may be called via contract or L1-L2 bot.

However, the deposit whitelist should ideally be enforced on the _user address. If msgSender() is blacklisted, user address can still deposit() from another whitelisted msgSender() address while retaining the user address that is used for leader boards and NFTs.

Impact: User misbehaves in interactions with the system (e.g. trolls, spams) and their corresponding msgSender() is removed from the whitelist. User continues to deposit into the system via another whitelisted msgSender() without any impact to leader boards or NFTs.

Proof of Concept

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L70-L71

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L204-L221

https://github.com/code-423n4/2021-06-realitycards/blob/86a816abb058cc0ed9b6f5c4a8ad146f22b8034c/contracts/RCTreasury.sol#L279-L297

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use whitelist on user address instead of msgSender().

Splidge commented 3 years ago

It is stated that the whitelist will "only allow certain addresses to deposit" here and that toggleWhitelist() allows an address to deposit here.

I think that the whitelist is performing as intended, but thanks for this issue report as this could easily have been a larger issue.

We only plan to use the whitelist as a very rudimentary barrier just for the initial launch. I think that only allowing certain addresses to deposit is sufficient for now. Maybe if time allows I'll make the changes but changing the whitelist to allow the _user instead of the msgSender() would also block contracts and layer1->layer2 bot, so there'd need to be exceptions made for them. I'd rather not play about with sensitive functions at the last minute when we aren't going to be using the whitelist much anyway.

dmvt commented 3 years ago

Warden makes a good point. This could allow griefing of other parts of the system. If the barrier winds up being needed longer than expected or users act in unexpected ways, sponsor may wind up wishing they had reconsidered addressing this. Obviously, sponsor is free to ignore, but the issue seems to be a valid one with significant potential impact.