[NAZ-M2] Gov NFTs Can Be Stolen By Compromised Owner

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L138

Vulnerability details

Impact

The sendNFTs() function is meant to be called by the owner to retrieve Gov NFTs. However, a compromised owner can use this function to then steal Gov NFTs.

Proof of Concept

Mallory compromised an owner and uses sendNFTs() to steal all Gov NFTs.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider removing sendNFTs() or documenting the possibility of it being used in a malicious way.

[TriHaz]

We are aware of the centralization risks, initially, all contracts will have a multi-sig as owner to prevent a sole owner, later on a DAO could be the owner.

[c4-sponsor]

TriHaz marked the issue as sponsor acknowledged

[GalloDaSballo]

This looks invalid, sendNFTs calls govNFT.safeTransferMany

Which ultimately transfers from msg.sender to msg.sender

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/GovNFT.sol#L247-L248

            _transfer(_msgSender(), _to, _ids[i]);

However the finding was sent as an admin issue, so am considering scrapping

[GalloDaSballo]

After re-checking, msg.sender is going to be the Lock contract, meaning the sweep can indeed be performed

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[c4-judge]

GalloDaSballo marked the issue as satisfactory