Disapproved proposals can still be approved, leading to double voting. The impact of this is medium as it can skew the change of the price of tokenContracts, but still found in a permissioned function controlled by onlyOneOfRoles with priceFeeds.
Proof of Concept
In the disapproveUSDPrice function, the initial checks are
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();
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L177-L207
Vulnerability details
Impact
Disapproved proposals can still be approved, leading to double voting. The impact of this is medium as it can skew the change of the price of
tokenContracts
, but still found in a permissioned function controlled byonlyOneOfRoles
with priceFeeds.Proof of Concept
In the disapproveUSDPrice function, the initial checks are
as seen at https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L224-L231
We see, that approvals and disapprovals of msg.sender is checked before allowing a disapproval
But we check the same for
approveUSDPrice
functionas seen at https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L19-L200
There is no check for disapproval an can cause a feed to vote twice effectively skewing the outcome of the proposal
Tools Used
Manual Review
Recommended Mitigation Steps
Add a disapproval check for
approveUSDPrice
functionAssessed type
Other