Don't use ^, lock pragma to ensure compiler version. e.g. pragma solidity 0.7.6;
(Non) Change the ownerOf requirement to isAuthorizedForToken modifier
Impact
isAuthorizedForToken modifier is already defined in UniV3LpVault.sol. It is better to unify the privilege checking method and not use require(ownerOf[tokenId] == msg.sender, …) directly in functions.
Change the ownerOf requirement to isAuthorizedForToken modifier.
(Non) _become and _becomeImplementation is unused
Impact
Duality removed upgradeability of Comptrollers, thus eliminating the need of Unitroller as proxy. Due to the modification, functions including Comptroller._become and Comptroller._becomeImplementation that are written to support upgradeability is no longer required, and should be removed.
Summary
We list 1 low-critical finding and 3 non-critical findings here:
isAuthorizedForTokenmodifier_becomeand_becomeImplementationis unuseduniV3LpVaultIn conclusion, it's better to remove unused functions, remove duplicate definitions, and use defined modifiers when forking 3rd party contracts.
(Low) Lock pragma to ensure compiler version
Impact
Floating pragma may cause unexpected compilation time behaviour and introduce unintended bugs.
Proof of Concept
https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/FlashLoan.sol#L1 https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/MasterPriceOracle.sol#L2 https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/UniV3LpVault.sol#L1 https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/UniswapTwapOracle.sol#L1
Recommended Mitigation Steps
Don't use
^, lock pragma to ensure compiler version. e.g.pragma solidity 0.7.6;(Non) Change the ownerOf requirement to
isAuthorizedForTokenmodifierImpact
isAuthorizedForTokenmodifier is already defined in UniV3LpVault.sol. It is better to unify the privilege checking method and not userequire(ownerOf[tokenId] == msg.sender, …)directly in functions.Proof of Concept
https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/vault_and_oracles/UniV3LpVault.sol#L102
Recommended Mitigation Steps
Change the ownerOf requirement to
isAuthorizedForTokenmodifier.(Non)
_becomeand_becomeImplementationis unusedImpact
Duality removed upgradeability of Comptrollers, thus eliminating the need of Unitroller as proxy. Due to the modification, functions including
Comptroller._becomeandComptroller._becomeImplementationthat are written to support upgradeability is no longer required, and should be removed.Proof of Concept
https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/compound_rari_fork/Comptroller.sol#L1445-L1459
Recommended Mitigation Steps
Remove
_becomeand_becomeImplementationfunctions.(Non) Duplicate definition of
uniV3LpVaultImpact
ComptrollerinheritsComptrollerV3StorageandComptrollerInterface, but both contracts have defineduniV3LpVault.Proof of Concept
https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/compound_rari_fork/Comptroller.sol#L18 https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/compound_rari_fork/ComptrollerStorage.sol#L46 https://github.com/code-423n4/2022-04-dualityfocus/blob/main/contracts/compound_rari_fork/ComptrollerInterface.sol#L8
Recommended Mitigation Steps
Remove duplicate definition of
uniV3LpVault.