baal audit notes

# baal audit notes see this for audit https://discord.com/channels/709210493549674598/953678109599739924/1008770690779983934 ## All Changes https://github.com/HausDAO/Baal/compare/fd4d14e0313276fc40479009174854e79dfd3800..d2a0ba15890cac061b89541f2ebfcec173852fb0 ## AUdit findings and actions taken ## **Critical** > No high severity issues were found. **High** 1. Library code should not be copied > Code from the popular OpenZeppelin library is copied into the > codebase. > This leads to an unnecessary increase in the audit scope and > introduces accidental change risks to otherwise safe and audited > code. > Paths: ./contracts/LootERC20.sol : transferFrom, name, symbol > ./contracts/SharesERC20.sol : transferFrom, name, symbol > Recommendation: Remove the copy-pasted code. Remove overriding for > transferFrom function. > Status: New ### Action Taken - Removed the transferFrom override from - ./contracts/LootERC20.sol - ./contracts/SharesERC20.sol - Renamed name and symbol to avoid shadowed variable names - The variables and the name() and symbol() overrides are needed because of being set in the setUp() initialization function - Replaced permit functions with OZ imports. Changed function signatures to take 'r,s,v' instead of 'signature'. - moved the vote/delegation functions in SharesERC20.sol to a new abstract in BaalVotes.sol. ## **Medium** 1. Denial of Service vulnerability > External calls can fail accidentally or deliberately, which can cause > a DoS condition in the contract. Inside the Baal’s totalSupply > function, there are two external calls ‘lootToken.tokenSupply’ and > ‘sharesToken.tokenSupply’. > This can lead to DoS condition in the contract > Path: ./contracts/Baal.sol : totalSupply() > Recommendation: Isolate each external call into its own transaction > that can be initiated by the recipient of the call. > Status: New ### Action Taken - LootToken and Shares Token are trusted contracts deployed in the initializer setup() function of Baal.sol - currently the tokens can not be ejected or changed - templates in factory constructor - there is no way to update them 2. Assembly usage > CloneFactory implements ‘createClone’ functionality using assembly. > Assembly usage can lead to error in implementation. > Path: ./contracts/Baal.sol : CloneFactory:createClone(address) > Recommendation: Use clone functionality from OpenZeppelin library. > Status: New ### Action Taken - Replaced with the OZ Clones import ## **Low** 1. Floating pragma > Locking the pragma helps ensure that contracts do not accidentally > get deployed using, for example, an outdated compiler version that > might introduce bugs that affect the contract system negatively. > The project uses floating pragmas 0.8.0. > Paths: ./contracts/Baal.sol > ./contracts/LootERC20.sol > ./contracts/SharesERC20.sol > ./contracts/interfaces/IBaal.sol > ./contracts/mock/MockBaal.sol > ./contracts/mock/TestAvatar.sol > ./contracts/mock/TestERC20.sol > ./contracts/tools/Poster.sol > ./contracts/tools/TributeMinionr.sol > Recommendation: Consider locking the pragma version whenever possible > and avoid using a floating pragma in the final deployment. > ### Action Taken - updated files to a static pragma 8.0.13 2. State variable default visibility > Labeling the visibility explicitly makes it easier to catch incorrect > assumptions about who can access the variable. > Paths: ./contracts/Baal.sol : status, multisendLibrary, > gnosisSafeProxyFactory, moduleProxyFactory > ./contracts/tools/TributeMinion.sol : escrow > Recommendation: Variables can be specified as being public, internal, > or private. Explicitly define visibility for all state variables. > Status: New ### Action Taken added explicit visibility - used slither to detect visibility changes. 3. Variable shadowing > Solidity allows for ambiguous naming of state variables when > inheritance is used. > Loot’s and Shares state variables ‘_name’ and ‘_symbol’ shadow ERC20 > state variables. > Paths: ./contracts/LootERC20.sol : _name, _symbol > ./contracts/SharesERC20.sol : _name, _symbol > Recommendation: Rename related variables/arguments. > Status: New ### Action Taken renamed variables to __name, __symbols 4. Commented code parts > Commented parts of code in a contract. They will not cause any > security issues, but make code less clear. > In the contracts : Shares (lines 116-120, 125, 137, 233) > TributeMinion (lines 5, 126, 133, 138), Baal (lines 381, 1164) are > commented parts of code. > This reduces code quality. > Paths: ./contracts/LootERC20.sol > ./contracts/SharesERC20.sol > ./contracts/Baal.sol > Recommendation: Remove commented parts of code. > Status: New ### Action Taken Removed commented code from these files 5. Unused variable > Unused variables should be removed from the contracts. Unused > variables are allowed in Solidity and do not pose a direct security > issue. It is best practice to avoid them as they can cause an > increase in computations (and unnecessary Gas consumption) and > decrease the readability. > The variable ‘nonces’ is never used inside the Baal contract. > Path: ./contracts/Baal.sol > State variable : nonces > Recommendation: Remove unused variables. > Status: New ### Action Taken Removed unused variables 6. Redundant import The use of unnecessary imports will increase the Gas consumption of the code. Thus they should be removed from the code. The second usage of Enum.sol is unnecessary for the Baal.sol contract. Path: ./contracts/Baal.sol Import: "@gnosis.pm/safe-contracts/contracts/common/Enum.sol" Recommendation: Remove the duplicate import. Status: New ### Action Taken removed duplicate imports 7. Missing zero address validation > Address parameters inside the BaalSammoner contract are being used > without checking against the possibility of 0x0 > This can lead to unwanted external calls to 0x0. > Path: ./contracts/Baal.sol > Constructors: _lootSingleton, _sharesSingleton, _gnosisSingleton > Recommendation: Remove the duplicate import. > Status: New ### Action Taken Removed duplicates --- ## Readme update - readme updated with more general notes - build command added to setup instructions to fix type issues with tests ## Folder structure updates - contracts and interfaces moved to their own files ## Issue copied OZ code in tokens - updated token contracts to use OZ imports - required an update to tests because of different OZ Permit function signatures - moved delegate and vote functions into a new abstract baalVotes.sol