0xJem
commented
7 months ago Invalid.
An auction lot can have the auction and derivative types defined, which are of type Keycode. When the auction lot is created, the latest version of the respective module is resolved, and the versioned reference (Veecode) to that module is stored on the auction lot.
Thus, even if the auction/derivative module is updated later, the specific version of the auction/derivative module will continue to be called for that particular auction.
FindEverythingX
medium
Upgrade of module will completely break synchronization and result in lost funds
Summary
Upgrade of module will completely break synchronization and result in lost funds
Vulnerability Detail
Contract: Modules.sol
Within the WithModule contract, the installModule function allows to install or upgrade modules:
https://github.com/sherlock-audit/2024-03-axis-finance/blob/main/moonraker/src/modules/Modules.sol#L160
Notably, if is only possible to upgrade modules with a version that is 1 larger than the current one:
if (version != status.latestVersion + 1)
The large problem here is that the important storage is held within the AuctionModule, once that is updated, the following calls will revert and result in locked funds:
a) cancel: Will revert due to invalid lotId b) purchase: Will revert due to invalid lotId c) curate: Module will be marked as ended due to conclusion for lotId = 0 d) bid: lotId is invalid
…..
At this point, one can already see the impact without the need to further elaborate which functions are reverting.
The root-cause is that the storage in the updated module will not match the storage in the previous module, while the storage in the AuctionHouse under all circumstances expects the correct synchronization.
This will result in a loss of all unsold funds for the FPAM module and a loss of all existing bids and funding for the EMPAM module.
I acknowledge that governance interactions may not fall under Sherlock’s bug conditions. However, I am of the very strong opinion that in such a situation where the architecture is definitely not fully thought out and this is a valid bug which must and will be fixed by the protocol.
In my opinion, there is absolutely no possibility where this logic can be deployed, as it is simply not practical. This aligns with Sherlock’s rules:
Direct Protocol Owner/Admin rug pulls:
“or if there is no scenario where a permissioned funtion can be used properly”
Moreover, it is not possible to install the old module again due to the version check.
The same issue applies to the sunsetModule function, as sunsetting cannot be reverted
Impact
IMPACT:
a) All bids will be locked/lost (bid storage is handled within AuctionModule (which is upgraded) ; “bids[lotId][bidId] => bid) b) Prefunded funds will be lost (can be circumvented by upgrading to dummy AuctionModule because storage in AuctionHouse stores capacity)
Code Snippet
https://github.com/sherlock-audit/2024-03-axis-finance/blob/main/moonraker/src/modules/Modules.sol#L160
Tool used
Manual Review
Recommendation
A refactoring of this logic is needed, in my opinion it should be simply removed and for upgrades a new deployment should be made. The root-cause of this issue is that a permanent synchronization is expected between AuctionHouse <-> AuctionModule.