Munchables - brevis's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L225-L226

Vulnerability details

Impact

If an approver approves the USD Price (by means of approveUSDPrice function), and later they change their mind and attempt to disapprove it (by means of disapproveUSDPrice), they will not be able to do so, thus leading to approval outcomes which don’t reflect the approver’s real intention.

Proof of concept

An approver approves the USD Price by calling the approveUSDPrice function.

// LockManager.sol

function approveUSDPrice(uint256 _price) ... {
	...
	usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
	...
}

Later on, they realize that the approval was erroneous and in fact the intention was to disapprove the USD price (or they just simply change their mind). They call the disapproveUSDPrice function expecting that the USD price will be disapproved. However, the function reverts as the value of usdUpdateProposal.disapprovals[msg.sender] is already set with _usdProposalId for this particular approver.

// LockManager.sol

function disapproveUSDPrice(uint256 _price) ... {
	...
	// the function reverts on the following line
	if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
		  revert ProposalAlreadyApprovedError();
        ...
	// the following line will not be executed
	usdUpdateProposal.disapprovals[msg.sender] = _usdProposalId;
	...
}

Tools Used

• Manual checking

Recommended Mitigation Steps

Remove the respective check:

// LockManager.sol

function disapproveUSDPrice(uint256 _price) ... {
	...
-	if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
-		  revert ProposalAlreadyApprovedError();
	...
}

Assessed type

Other