search

aavegotchi / aavegotchi-realm-diamond

23 stars 9 forks source link

[Audit Report] [N3] [Suggestion] Missing event records #17

Closed orionstardust closed 2 years ago

orionstardust commented 2 years ago

Description

  1. In the OwnerFunctions of the InstallationFacet contract, the owner role can set the aavegotchi diamond address, the Realm diamond address, and the $GLMR token address through the setAddresses function, add an installation type through the addInstallationTypes function, and edit an installationType through the editInstallationType function. And there are no event logging is preformed.

  2. In the RealmFacet contract, the owner role can set diamond address for Baazaar through the setAavegotchiDiamond function and the InstallationDiamond can upgrade Installation info through the upgradeInstallation function.And there are no event logging is preformed.

  3. In the AlchemicaFacet contract, the owner can add surveyingRound through the progressSurveyingRound function and set some important diamond state variables through the setVars function. And there are no event logging is preformed.

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/cee38d37307c49dc41cdb737a962d5d313c1cd4f/contracts/InstallationDiamond/facets/InstallationFacet.sol#L479

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/cee38d37307c49dc41cdb737a962d5d313c1cd4f/contracts/RealmDiamond/facets/RealmFacet.sol#L145

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/cee38d37307c49dc41cdb737a962d5d313c1cd4f/contracts/RealmDiamond/facets/RealmFacet.sol#L175

https://github.com/aavegotchi/aavegotchi-realm-diamond/blob/cee38d37307c49dc41cdb737a962d5d313c1cd4f/contracts/RealmDiamond/facets/AlchemicaFacet.sol#L76

Solution

It is recommended to record events when modifying sensitive parameters.

orionstardust commented 2 years ago

@mudgen @cinnabarhorse They suggested a lot of events.. What do you think about it? Which are important (sensitive) params, do you think?

cinnabarhorse commented 2 years ago

Let's add the following events:

AddressesUpdated() AavegotchiDiamondUpdated(address) InstallationUpgraded(uint256 _realmId, uint256 _prevInstallationId, uint256 _nextInstallationId, uint256 _coordinateX, uint256 _coordinateY) SurveyingRoundProgressed(uint256 _newRound)

What do you think? @orionstardust