if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
revert ProposalAlreadyApprovedError();
This line assumes that _usdProposalId is a unique identifier for each proposal, but it is not clear if this is the case. If _usdProposalId is not unique, it could lead to a situation where a proposal is incorrectly marked as already approved.
Proof of Concept
The impact of this is that if _usdProposalId is not unique for each proposal, it could lead to incorrect tracking of approvals and potentially allow a proposal to be approved or rejected incorrectly.
Explanation of the issue and the relevant code:
The approveUSDPrice function takes a single parameter _price of type uint256, which represents the proposed USD price.
Inside the function, there is a check to ensure that the sender (msg.sender) has not already approved the current proposal: src/managers/LockManager.sol#L194-L195
if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
revert ProposalAlreadyApprovedError();
The usdUpdateProposal.approvals is a mapping that stores the approval status of each address (msg.sender) for a specific proposal, identified by _usdProposalId.
The issue arises because the code assumes that _usdProposalId is a unique identifier for each proposal, but there is no clear indication or guarantee of this in the code.
If _usdProposalId is not unique for each proposal, it could lead to the following scenario:
Proposal A is created with a non-unique _usdProposalId value.
An address (e.g., priceFeed2) approves Proposal A.
Later, a different Proposal B is created with the same _usdProposalId value as Proposal A.
When priceFeed2 tries to approve Proposal B, the check usdUpdateProposal.approvals[msg.sender] == _usdProposalId will incorrectly identify it as already approved because the _usdProposalId value is the same as the previous proposal (Proposal A).
This incorrect tracking of approvals could lead to a proposal being approved or rejected incorrectly, violating the intended logic of the contract.
Tools Used
Manual Audit
Recommended Mitigation Steps
Use a unique identifier for each proposal (e.g., a hash of the proposal details) or tracking the proposals separately (e.g., using a mapping from proposal identifiers to proposal details).
Without a clear guarantee of the uniqueness of _usdProposalId for each proposal, the current implementation of the approveUSDPrice function is susceptible to incorrect tracking of approvals.
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L194-L195
Vulnerability details
Impact
src/managers/LockManager.sol#L194-L195
This line assumes that
_usdProposalIdis a unique identifier for each proposal, but it is not clear if this is the case. If_usdProposalIdis not unique, it could lead to a situation where a proposal is incorrectly marked as already approved.Proof of Concept
The impact of this is that if
_usdProposalIdis not unique for each proposal, it could lead to incorrect tracking of approvals and potentially allow a proposal to be approved or rejected incorrectly.Explanation of the issue and the relevant code:
The
approveUSDPricefunction takes a single parameter_priceof typeuint256, which represents the proposed USD price.Inside the function, there is a check to ensure that the sender (
msg.sender) has not already approved the current proposal: src/managers/LockManager.sol#L194-L195The
usdUpdateProposal.approvalsis a mapping that stores the approval status of each address (msg.sender) for a specific proposal, identified by_usdProposalId.The issue arises because the code assumes that
_usdProposalIdis a unique identifier for each proposal, but there is no clear indication or guarantee of this in the code.If
_usdProposalIdis not unique for each proposal, it could lead to the following scenario:Proposal A is created with a non-unique
_usdProposalIdvalue. An address (e.g.,priceFeed2) approves Proposal A. Later, a different Proposal B is created with the same_usdProposalIdvalue as Proposal A. When priceFeed2 tries to approve Proposal B, the checkusdUpdateProposal.approvals[msg.sender] == _usdProposalIdwill incorrectly identify it as already approved because the_usdProposalIdvalue is the same as the previous proposal (Proposal A).This incorrect tracking of approvals could lead to a proposal being approved or rejected incorrectly, violating the intended logic of the contract.
Tools Used
Manual Audit
Recommended Mitigation Steps
Use a unique identifier for each proposal (e.g., a hash of the proposal details) or tracking the proposals separately (e.g., using a mapping from proposal identifiers to proposal details).
Without a clear guarantee of the uniqueness of
_usdProposalIdfor each proposal, the current implementation of theapproveUSDPricefunction is susceptible to incorrect tracking of approvals.Assessed type
Error