Pricefeed role users should not be allowed to approve prices after disapproving it, this leads to a single role holder filling up both sides of the proposal threshold. #497
The approveUSDPrice() function allows price feed role holders to approve prices after disapproving as there is no check for it the caller has already voted to disapprove the role
function approveUSDPrice(
uint256 _price
)
external
onlyOneOfRoles(
[
Role.PriceFeed_1,
Role.PriceFeed_2,
Role.PriceFeed_3,
Role.PriceFeed_4,
Role.PriceFeed_5
]
)
{
if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
if (usdUpdateProposal.proposer == msg.sender)
revert ProposerCannotApproveError();
if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
revert ProposalAlreadyApprovedError();
if (usdUpdateProposal.proposedPrice != _price)
revert ProposalPriceNotMatchedError();
usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
usdUpdateProposal.approvalsCount++;
if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
_execUSDPriceUpdate();
}
emit ApprovedUSDPrice(msg.sender);
}
This breaks the whole system of voting as voters are required to pick a side and not support both sides
Impact
price feed role holders can vote to approve a price after rejecting or declining the proposed price thereby filling up both sides of the threshold and preventing other role holders from voting.
Tools Used
manual review
Recommended Mitigation Steps
The approveUSDPrice() function should be corrected to prevent role holder that have already disapproved the price from calling it
function approveUSDPrice(
uint256 _price
)
external
onlyOneOfRoles(
[
Role.PriceFeed_1,
Role.PriceFeed_2,
Role.PriceFeed_3,
Role.PriceFeed_4,
Role.PriceFeed_5
]
)
{
if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
if (usdUpdateProposal.proposer == msg.sender)
revert ProposerCannotApproveError();
++ if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
++ revert ProposalAlreadyDisapprovedError();
if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
revert ProposalAlreadyApprovedError();
if (usdUpdateProposal.proposedPrice != _price)
revert ProposalPriceNotMatchedError();
usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
usdUpdateProposal.approvalsCount++;
if (usdUpdateProposal.approvalsCount >= APPROVE_THRESHOLD) {
_execUSDPriceUpdate();
}
emit ApprovedUSDPrice(msg.sender);
}
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L177-L207
Vulnerability details
The approveUSDPrice() function allows price feed role holders to approve prices after disapproving as there is no check for it the caller has already voted to disapprove the role
This breaks the whole system of voting as voters are required to pick a side and not support both sides
Impact
price feed role holders can vote to approve a price after rejecting or declining the proposed price thereby filling up both sides of the
threshold
and preventing other role holders from voting.Tools Used
manual review
Recommended Mitigation Steps
The
approveUSDPrice()
function should be corrected to prevent role holder that have already disapproved the price from calling itAssessed type
Other