approveUSDPrice() does not check if the price approval is for the same contracts and assumes it should update for all of the proposed ones, but proposeUSDPrice does not emit contracts / tokens / that will be updated with the new USD price:
emit ProposedUSDPrice(msg.sender, _price);
This could lead to approving a wrong price for a token that should not be updated, because approveUSDPrice only has the _price as a function parameter. After reaching the approval threshold, all of the USDUpdateProposal.contracts are updated, even though the approvals just validated the price.
Updating multiple contracts with 1 USDPrice could be dangerous.
Impact
Approving the USD price proposed for multiple contracts can lead to invalid values for some contracts, thus leading over/under pricing of the USD value for the calculation in getLockedWeightedValue();
Tools Used
Manual review
Recommendation
The simplest way is to: update the USD price for 1 contract at a time.
Or emit the contracts and make the price proposers verify that they want to update the proposed USD price for all of them.
Or alternatively, include an additional function parameter in approveUSDPrice which also contains the contracts for which the price is being approved.
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L173 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L177 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L506-L526
Vulnerability details
Vulnerability Detail
approveUSDPrice() does not check if the price approval is for the same contracts and assumes it should update for all of the proposed ones, but proposeUSDPrice does not emit contracts / tokens / that will be updated with the new USD price:
emit ProposedUSDPrice(msg.sender, _price);
This could lead to approving a wrong price for a token that should not be updated, because approveUSDPrice only has the _price as a function parameter. After reaching the approval threshold, all of the USDUpdateProposal.contracts are updated, even though the approvals just validated the price. Updating multiple contracts with 1 USDPrice could be dangerous.
Impact
Approving the USD price proposed for multiple contracts can lead to invalid values for some contracts, thus leading over/under pricing of the USD value for the calculation in getLockedWeightedValue();
Tools Used
Manual review
Recommendation
The simplest way is to: update the USD price for 1 contract at a time.
Or emit the contracts and make the price proposers verify that they want to update the proposed USD price for all of them.
Or alternatively, include an additional function parameter in approveUSDPrice which also contains the contracts for which the price is being approved.
Assessed type
Context