approveUSDPrice is missing the validation for ProposalAlreadyDisapprovedError(). This can lead to
unintended USD price voting outcomes, as the msg.sender, can make an approved and also a disapproved vote,
essentially lowering the threshold for executing a proposal. There is no way to revoke approvals or disapprovals.
Proof of Concept
The disapproveUSDPrice validates if the caller has already approved the USD price.
if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
revert ProposalAlreadyApprovedError();
Because of this validation, the caller can not achieve a state with both an approval and a disapproval if first the
approveUSDPrice is called and then disapproveUSDPrice, as the disapproveUSDPrice will revert, and the
caller will end up only with an approval. However, this state can be achieved if first the disapproveUSDPrice is
called and then the approveUSDPrice, as the approveUSDPrice is missing the validation if the msg.sender has
already disapproved the USD price.
Tools Used
Manual review.
Recommended Mitigation Steps
Add validation to the approveUSDPrice if the msg.sender has already disapproved the USD price.
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.disapprovals[msg.sender] == _usdProposalId)
+ revert ProposalAlreadyDisapprovedError();
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/main/src/managers/LockManager.sol#L177-L207
Vulnerability details
Impact
approveUSDPrice
is missing the validation forProposalAlreadyDisapprovedError()
. This can lead to unintended USD price voting outcomes, as themsg.sender
, can make an approved and also a disapproved vote, essentially lowering the threshold for executing a proposal. There is no way to revoke approvals or disapprovals.Proof of Concept
The
disapproveUSDPrice
validates if the caller has already approved the USD price.Because of this validation, the caller can not achieve a state with both an approval and a disapproval if first the
approveUSDPrice
is called and thendisapproveUSDPrice
, as thedisapproveUSDPrice
will revert, and the caller will end up only with an approval. However, this state can be achieved if first thedisapproveUSDPrice
is called and then theapproveUSDPrice
, as theapproveUSDPrice
is missing the validation if themsg.sender
has already disapproved the USD price.Tools Used
Manual review.
Recommended Mitigation Steps
Add validation to the
approveUSDPrice
if themsg.sender
has already disapproved the USD price.Assessed type
Invalid Validation