function disapproveUSDPrice(
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.approvals[msg.sender] == _usdProposalId)
revert ProposalAlreadyApprovedError();
if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId)
revert ProposalAlreadyDisapprovedError();
if (usdUpdateProposal.proposedPrice != _price)
revert ProposalPriceNotMatchedError();
usdUpdateProposal.disapprovalsCount++;
usdUpdateProposal.disapprovals[msg.sender] = _usdProposalId;
emit DisapprovedUSDPrice(msg.sender);
if (usdUpdateProposal.disapprovalsCount >= DISAPPROVE_THRESHOLD) {
delete usdUpdateProposal;
emit RemovedUSDProposal();
}
}
Here it is observed that Role.PriceFeed is not allowed to call this function after calling the approveUSDPrice function due to ProposalAlreadyApprovedError revert.
This eventually increases the disapprovalsCount.
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);
}
Here, it is observed that the same Role.PrceFeed can call the approveUSDPrice function after calling the disapproveUSDPrice function due to lack of validation. This eventually increases the approvalsCount unethically as a single Role.PriceFeed can both disapprove as well as approve a proposal.
This means that a Role.PriceFeed cannot approve a proposal and then disprove the same proposal however a Role.PriceFeed can first disapprove and then approve the same proposal.
This leads to a waste of vote or favor towards a particular decision.
Tools Used
Manual Review
Recommended Mitigation Steps
Add this check in the approveUSDPrice function:
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#L210-L242 https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L177-L207
Vulnerability details
Impact
It is possible for a Role.PriceFeed to both approve and disapprove the same proposal unethically.
Proof of Concept
According to the contest page,
Role.PriceFeed
is not a trusted role. In thedisapproveUSDPrice
function: https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L210-L242Here it is observed that Role.PriceFeed is not allowed to call this function after calling the
approveUSDPrice
function due toProposalAlreadyApprovedError
revert. This eventually increases thedisapprovalsCount
.However in the
approveUSDPrice
function:https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L177-L207
Here, it is observed that the same Role.PrceFeed can call the
approveUSDPrice
function after calling thedisapproveUSDPrice
function due to lack of validation. This eventually increases theapprovalsCount
unethically as a single Role.PriceFeed can both disapprove as well as approve a proposal.This means that a Role.PriceFeed cannot approve a proposal and then disprove the same proposal however a Role.PriceFeed can first disapprove and then approve the same proposal.
This leads to a waste of vote or favor towards a particular decision.
Tools Used
Manual Review
Recommended Mitigation Steps
Add this check in the
approveUSDPrice
function:Assessed type
Invalid Validation