The contract allows a PriceFeed to both approve and disapprove the same USD price proposal, leading to an inconsistent and invalid state where a single entity has conflicting actions on the same proposal.
Proof of Concept
In LockManager.sol:177, the approveUSDPrice function does not check if the msg.sender has previously disapproved the proposal before allowing them to approve it. The contract already has a disapprovals mapping to track disapprovals, but this mapping is not being checked in the approveUSDPrice function. This means that a PriceFeed can potentially both approve and disapprove the same proposal, leading to an inconsistent state.
Tools Used
Manual Review
Recommended Mitigation Steps
To prevent this inconsistency, the approveUSDPrice function should check the disapprovals mapping to ensure that the msg.sender has not previously disapproved the proposal. If they have, the function should revert to maintain consistency.
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 (disapprovals[msg.sender] == _usdProposalId) revert ProposalAlreadyDisapprovedError();
if (usdUpdateProposal.proposedPrice != _price) revert ProposalPriceNotMatchedError();
usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
emit ProposalApproved(msg.sender, _price);
// ...
}
By checking the disapprovals mapping, the function will ensure that a PriceFeed cannot both approve and disapprove the same proposal, maintaining consistency in the contract's state.
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L177
Vulnerability details
Impact
The contract allows a PriceFeed to both approve and disapprove the same USD price proposal, leading to an inconsistent and invalid state where a single entity has conflicting actions on the same proposal.
Proof of Concept
In
LockManager.sol:177
, theapproveUSDPrice
function does not check if themsg.sender
has previously disapproved the proposal before allowing them to approve it. The contract already has adisapprovals
mapping to track disapprovals, but this mapping is not being checked in theapproveUSDPrice
function. This means that a PriceFeed can potentially both approve and disapprove the same proposal, leading to an inconsistent state.Tools Used
Manual Review
Recommended Mitigation Steps
To prevent this inconsistency, the
approveUSDPrice
function should check thedisapprovals
mapping to ensure that themsg.sender
has not previously disapproved the proposal. If they have, the function should revert to maintain consistency.By checking the
disapprovals
mapping, the function will ensure that a PriceFeed cannot both approve and disapprove the same proposal, maintaining consistency in the contract's state.Issue Type
Governance
Assessed type
Governance