JuggerNaut63 - Replay Attack in VoteDelegateFactory Contract Creation Mechanism

[sherlock-admin4]

JuggerNaut63

Medium

Replay Attack in VoteDelegateFactory Contract Creation Mechanism

Summary

create function uses a deterministic salt derived from msg.sender for the create2 opcode. This approach can lead to replay attacks, where the same user can only create one VoteDelegate contract, and any subsequent attempts will fail due to address collision.

Vulnerability Detail

Issue: The create function uses create2 with a salt derived from msg.sender:

voteDelegate = address(new VoteDelegate{salt: bytes32(uint256(uint160(msg.sender)))}(chief, polling, msg.sender));

Problem: If the same user (msg.sender) calls create more than once, the salt remains the same, leading to the same contract address being generated. Since the address is already occupied by the first contract creation, subsequent calls will fail.

Impact

• Denial of Service
• User Experience

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/vote-delegate/src/VoteDelegateFactory.sol#L61-L66

Tool used

Manual Review

Recommendation

• Ensure that a VoteDelegate does not already exist for the user before allowing the creation of a new one. This prevents the replay attack by ensuring only one VoteDelegate per user.

  
  function create() external returns (address voteDelegate) {

• require(created[getAddress(msg.sender)] == 0, "VoteDelegate already exists"); voteDelegate = address(new VoteDelegate{salt: bytes32(uint256(uint160(msg.sender)))}(chief, polling, msg.sender)); created[voteDelegate] = 1;

  emit CreateVoteDelegate(msg.sender, voteDelegate); }

• Incorporate a unique value, such as a nonce, to ensure each create2 call generates a unique address, even for the same user.

• function create() external returns (address voteDelegate) {
• voteDelegate = address(new VoteDelegate{salt: bytes32(uint256(uint160(msg.sender)))}(chief, polling, msg.sender));
• function create(uint256 nonce) external returns (address voteDelegate) {
• bytes32 salt = keccak256(abi.encodePacked(msg.sender, nonce));

• voteDelegate = address(new VoteDelegate{salt: salt}(chief, polling, msg.sender)); created[voteDelegate] = 1;

  emit CreateVoteDelegate(msg.sender, voteDelegate); }

• Maintain a nonce for each user to ensure unique salts without requiring the user to provide a nonce.

• mapping(address => uint256) private nonces;

function create() external returns (address voteDelegate) {

• voteDelegate = address(new VoteDelegate{salt: bytes32(uint256(uint160(msg.sender)))}(chief, polling, msg.sender));
• uint256 nonce = nonces[msg.sender]++;
• bytes32 salt = keccak256(abi.encodePacked(msg.sender, nonce));

• voteDelegate = address(new VoteDelegate{salt: salt}(chief, polling, msg.sender)); created[voteDelegate] = 1;

  emit CreateVoteDelegate(msg.sender, voteDelegate); }

[sunbreak1211]

First of all the usage of "Replay Attack" here is pretty weird as it doesn't seem it is understood what it means. Secondly this is not a bug, if the contract was already created, it is ok to revert when trying to do it again. There is not a denial of service at all, the msg.sender already has a voteDelegate created.