Attacker can make 0 msg.value receive() function calls to spam the whitelist

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/hardhat-vultisig/contracts/Whitelist.sol#L65

Vulnerability details

Summary

Context: The receive() function in the Whitelist.sol contract is called by users to self-whitelist themselves when _isSelfWhitelistDisabled is false and provided they are not blacklisted.

Issue: Currently though, this mechanism is prone to spamming/griefing since an attacker can make 0 msg.value (no upfront cost other than gas) or dust amount of msg.value calls to the receive() function.

Impact: Since there are expected to be 1k whitelist slots as per the README, the attacker can spam the whitelist with random addresses at any point in time. The impact of this is that:

1. The invariants stated at the top of the contract here and in the README in the 8th bullet point from the top here is broken since the attacker can make a 0 msg.value call.
2. The attack can be performed at different intervals since it could be done from multiple addresses easily to fool the team into believing they're real users.
3. Even if the team identifies the spam, they would not be aware of the number of spam addresses by which the _allowedWhitelistIndex needs to be extended and the addresses to blacklist.

Basically, as long as the self-whitelist is open, the attack can be performed with ease. This eventually breaks the whitelist functionality and introduces uncertainty as to whether the whitelist is full of spammers or not.

Even if we assume another round of self-whitelist is held without the spammer being involved, there would a certain set of legitimate users who miss out on a spot on the second try due to the limited spots. Overall, whitelisting in this manner is unguarded.

Proof of Concept

1. Owner of Whitelist.sol enables the self-whitelist by setting _isSelfWhitelistDisabled to false.

File: Whitelist.sol
104:     function isSelfWhitelistDisabled() external view returns (bool) {
105:         return _isSelfWhitelistDisabled;
106:     }
107: 

2. Attacker calls the receive() function with 0 msg.value (no upfront cost other than gas) with X number of spam addresses in Y intervals to make it look natural.
   • Line 70 - Since this was set to false, we continue.
   • Line 73 - We assume attacker spam address is not blacklisted.
   • Line 76 - Adds the spam address to whitelist
   • Line 78 - Transfer 0 msg.value.

     File: Whitelist.sol
     69:     receive() external payable {
     70:         if (_isSelfWhitelistDisabled) {
     71:             revert SelfWhitelistDisabled();
     72:         }
     73:         if (_isBlacklisted[_msgSender()]) {
     74:             revert Blacklisted();
     75:         }
     76:         _addWhitelistedAddress(_msgSender());
     77:       
     78:         payable(_msgSender()).transfer(msg.value);
     79:     }

Here is a simple POC to prove 0 msg.value calls go through:

• Add to Remix in sample file
• Deploy the contract and read the value of x to be 0.
• Call transact at the bottom without any msg.data.
• Read the value of x to be 10.

  
  // SPDX-License-Identifier: GPL-3.0-only
  pragma solidity 0.8.19;

contract ContractG {

uint256 public x;

receive() external payable {
    x = 10;
    payable(msg.sender).transfer(msg.value);
}

}

## Tools Used
Manual Review

## Recommended Mitigation Steps
The issue cannot be solved by introducing a 0 msg.value check or even a minimum threshold since the msg.value is refunded in the end anyways. The simplest solution would be to remove the self-whitelist mechanism and only allow the owner (a multisig) to add users to the whitelist. 

## Assessed type

DoS

[mcgrathcoutinho]

Hey @alex-ppg, could you please review this issue?

I think it is important to realize the impacts this issue would have, which have been stated in the Summary section. I think it is of Medium-severity risk since as explained it introduces uncertainty in the number of real whitelisted addresses existing within the limited 1k slots as well as how other users could get affected as well.

I'd like to hear your views on this, thank you!

[alex-ppg]

Hey @mcgrathcoutinho, thank you for the PJQA contribution. I will preface all validation repository finding responses by stating that they are not evaluated by judges directly and are only evaluated by the validators if they are deemed unsatisfactory.

An attacker cannot make repetitive calls via the same address due to this code block, and an attack that exhausts the list is unrealistic if we consider that a new address needs to interact each time.

This paragraph is included in all of my responses and confirms that no further feedback is expected in this submission as PJQA has concluded. You are free to refute any of my statements factually, however, I strongly implore you to do this with actual code references and examples.