Open code423n4 opened 2 years ago
https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L102 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L110 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L134 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L152 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L161 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L169
Users are unaware of important changes in the protocol.
While event emission are mere informative in most cases. There are other parameters changed by admins that are crucial for users to know.
Timelocks, in my opinion, are ineffective without event emissions because the purpose of a timelock is to give users enough time to make decisions.
proposeNewBasketImplementation()
updateBasketImplementation()
An evil admin could update the implementation to steal users tokens in the basket
proposeNewVaultImplementation()
updateVaultImplementation()
User are unaware of new paid fees
Add the events and emit them
Historically events findings are Informational / Non-critical
Agree. Lowering the risk and converting this into a QA report for the warden.
Lines of code
https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L102 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L110 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L134 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L152 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L161 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVaultFactory.sol#L169
Vulnerability details
Impact
Users are unaware of important changes in the protocol.
Poc
While event emission are mere informative in most cases. There are other parameters changed by admins that are crucial for users to know.
Timelocks, in my opinion, are ineffective without event emissions because the purpose of a timelock is to give users enough time to make decisions.
In
proposeNewBasketImplementation()
andupdateBasketImplementation()
An evil admin could update the implementation to steal users tokens in the basket
In
proposeNewVaultImplementation()
andupdateVaultImplementation()
At proposeNewAdminFee() and updateNewAdminFee()
User are unaware of new paid fees
Recommended
Add the events and emit them