A big aspect of a 2-step change, such as done with changePublisher(), is to mitigate the risk from accidentally setting them to addresses for which private keys are not available. This is typically done by requiring the second transaction to “claim” the pending role (that was granted/approved in the first transaction) by requiring the msg.sender to be that granted/approved address.
The current implementation of changePublisher can therefore be made more fault-tolerant by requiring the msg.sender for claiming to be the pendingPublisher.publisher address in a separate claimNewPublisher() function. This will prevent changing publisher address to an incorrect one for which private keys are not available.
Split changePublisher() into two functions setPendingPublisher() which is onlyPublisher that sets pendingPublisher.publisher to the supplied newPublisher address and another claimPendingPublisher() where there is a check that msg.sender == pendingPublisher.publisher and that is allowed to execute only after timelock consdition is met i.e. the logic in the if part of the conditional currently.
GalloDaSballo
commented
2 years ago
Handle
0xRajeev
Vulnerability details
Impact
A big aspect of a 2-step change, such as done with changePublisher(), is to mitigate the risk from accidentally setting them to addresses for which private keys are not available. This is typically done by requiring the second transaction to “claim” the pending role (that was granted/approved in the first transaction) by requiring the msg.sender to be that granted/approved address.
The current implementation of changePublisher can therefore be made more fault-tolerant by requiring the msg.sender for claiming to be the pendingPublisher.publisher address in a separate claimNewPublisher() function. This will prevent changing publisher address to an incorrect one for which private keys are not available.
Proof of Concept
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Basket.sol#L133-L148
Tools Used
Manual Analysis
Recommended Mitigation Steps
Split changePublisher() into two functions setPendingPublisher() which is onlyPublisher that sets pendingPublisher.publisher to the supplied newPublisher address and another claimPendingPublisher() where there is a check that msg.sender == pendingPublisher.publisher and that is allowed to execute only after timelock consdition is met i.e. the logic in the if part of the conditional currently.