When dealing with usd price proposals, price feeds ought to have two options:
vote to approve or to disapprove a proposed price.
This is not enforced on its entirety at the LockManager contract, however.
The disapproveUSDPrice function properly enforces that by checking whether the price feed hasn't approved nor disapproved a proposed price before accounting the disapprove vote:
function disapproveUSDPrice(
uint256 _price
)
...
if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
revert ProposalAlreadyApprovedError();
if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
revert ProposalAlreadyDisapprovedError();
...
The approveUSDPrice doesn't enforce the same checks. It only verifies the price feed hasn't approved the price priorly:
function approveUSDPrice(
uint256 _price
)
...
if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
revert ProposalAlreadyApprovedError();
...
Impact
The current price approval mechanism is not fair on all price fees.
This issue allows privilege escalation as a price feed is able to vote twice.
Allowing one voter to vote on both approval and disapproval exceeds the intended permissions by creating the possibility of voting ties: the system has 5 price feeds so that ties are impossible, but this privilege escalation allows both for and against positions to have 3 or more votes. This enables an unexpected logic for price approvals - price feeds will have incentives to vote as soon as possible.
Tools Used
Manual review
Recommended Mitigation Steps
Add the following check to approveUSDPrice:
if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
revert ProposalAlreadyDisapprovedError();
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L194 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L227
Vulnerability details
Description
When dealing with usd price proposals, price feeds ought to have two options: vote to approve or to disapprove a proposed price. This is not enforced on its entirety at the LockManager contract, however. The disapproveUSDPrice function properly enforces that by checking whether the price feed hasn't approved nor disapproved a proposed price before accounting the disapprove vote:
The approveUSDPrice doesn't enforce the same checks. It only verifies the price feed hasn't approved the price priorly:
Impact
The current price approval mechanism is not fair on all price fees. This issue allows privilege escalation as a price feed is able to vote twice.
Allowing one voter to vote on both approval and disapproval exceeds the intended permissions by creating the possibility of voting ties: the system has 5 price feeds so that ties are impossible, but this privilege escalation allows both for and against positions to have 3 or more votes. This enables an unexpected logic for price approvals - price feeds will have incentives to vote as soon as possible.
Tools Used
Manual review
Recommended Mitigation Steps
Add the following check to approveUSDPrice:
Assessed type
Other