-
Notifications
You must be signed in to change notification settings - Fork 2
refactor: use forge lint over solhint and adjust code accordingly
#63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,6 @@ | |
| }, | ||
| "devDependencies": { | ||
| "prettier": "^3.0.0", | ||
| "solhint-community": "^3.6.0", | ||
| "commit-and-tag-version": "^12.2.0" | ||
| }, | ||
| "keywords": [ | ||
|
|
@@ -29,7 +28,7 @@ | |
| "verify:mp_less_equal_max_mp": "certoraRun certora/confs/MPLessEqualMaxMP.conf", | ||
| "verify:emergency_mode": "certoraRun certora/confs/EmergencyMode.conf", | ||
| "verify:karma": "certoraRun certora/confs/Karma.conf", | ||
| "lint:sol": "forge fmt --check && pnpm solhint {script,src,test,certora}/**/*.sol", | ||
| "lint:sol": "forge fmt --check && forge lint", | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CI and |
||
| "prettier:check": "prettier --check **/*.{json,md,yml} --ignore-path=.prettierignore", | ||
| "prettier:write": "prettier --write **/*.{json,md,yml} --ignore-path=.prettierignore", | ||
| "gas-report": "forge snapshot --gas-report 2>&1 | (tee /dev/tty | awk '/Suite result:/ {found=1; buffer=\"\"; next} found && !/Ran/ {buffer=buffer $0 ORS} /Ran/ {found=0} END {printf \"%s\", buffer}' > .gas-report)", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ pragma solidity 0.8.26; | |
| import { AccessControlUpgradeable } from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; | ||
| import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; | ||
| import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; | ||
| import { ERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol"; | ||
| import { ERC20VotesUpgradeable } from "./utils/ERC20VotesUpgradeable.sol"; | ||
| import { IRewardDistributor } from "./interfaces/IRewardDistributor.sol"; | ||
| import { EnumerableSet } from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; | ||
|
|
@@ -88,17 +87,13 @@ contract Karma is Initializable, ERC20VotesUpgradeable, UUPSUpgradeable, AccessC | |
|
|
||
| /// @notice Modifier to check if sender is admin or operator | ||
| modifier onlyAdminOrOperator() { | ||
| if (!hasRole(DEFAULT_ADMIN_ROLE, msg.sender) && !hasRole(OPERATOR_ROLE, msg.sender)) { | ||
| revert Karma__Unauthorized(); | ||
| } | ||
| _onlyAdminOrOperator(msg.sender); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Modifier functionality has been moved into internal functions, because otherwise, the logic gets inlined into every function where the modifier is used. |
||
| _; | ||
| } | ||
|
|
||
| /// @notice Modifier to check if sender has slasher role | ||
| modifier onlySlasher() { | ||
| if (!hasRole(DEFAULT_ADMIN_ROLE, msg.sender) && !hasRole(SLASHER_ROLE, msg.sender)) { | ||
| revert Karma__Unauthorized(); | ||
| } | ||
| _onlySlasher(msg.sender); | ||
| _; | ||
| } | ||
|
|
||
|
|
@@ -397,6 +392,18 @@ contract Karma is Initializable, ERC20VotesUpgradeable, UUPSUpgradeable, AccessC | |
| return (super.balanceOf(account) + externalBalance); | ||
| } | ||
|
|
||
| function _onlyAdminOrOperator(address sender) internal view { | ||
| if (!hasRole(DEFAULT_ADMIN_ROLE, sender) && !hasRole(OPERATOR_ROLE, sender)) { | ||
| revert Karma__Unauthorized(); | ||
| } | ||
| } | ||
|
|
||
| function _onlySlasher(address sender) internal view { | ||
| if (!hasRole(DEFAULT_ADMIN_ROLE, sender) && !hasRole(SLASHER_ROLE, sender)) { | ||
| revert Karma__Unauthorized(); | ||
| } | ||
| } | ||
|
|
||
| /*////////////////////////////////////////////////////////////////////////// | ||
| VIEW FUNCTIONS | ||
| //////////////////////////////////////////////////////////////////////////*/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,39 +33,41 @@ contract KarmaAirdrop is Ownable2Step, Pausable { | |
| event MerkleRootSet(bytes32 merkleRoot); | ||
|
|
||
| /// @notice The address of the Karma token contract | ||
| address public immutable token; | ||
| address public immutable TOKEN; | ||
| /// @notice Whether the merkle root can be updated more than once | ||
| bool public immutable allowMerkleRootUpdate; | ||
| bool public immutable ALLOW_MERKLE_ROOT_UPDATE; | ||
| /// @notice The default delegatee address for new claimers | ||
| address public immutable defaultDelegatee; | ||
| address public immutable DEFAULT_DELEGATEE; | ||
| /// @notice The Merkle root of the airdrop | ||
| bytes32 public merkleRoot; | ||
| /// @notice Current epoch - incremented with each merkle root update | ||
| uint256 public epoch; | ||
| /// @notice A bitmap to track claimed indices per epoch | ||
| mapping(uint256 => mapping(uint256 => uint256)) private claimedBitMap; | ||
| /// @notice Base value for creating bitmap masks | ||
| uint256 private constant BITMAP_MASK_BASE = 1; | ||
|
|
||
| constructor(address _token, address _owner, bool _allowMerkleRootUpdate, address _defaultDelegatee) { | ||
| token = _token; | ||
| allowMerkleRootUpdate = _allowMerkleRootUpdate; | ||
| defaultDelegatee = _defaultDelegatee; | ||
| TOKEN = _token; | ||
| ALLOW_MERKLE_ROOT_UPDATE = _allowMerkleRootUpdate; | ||
| DEFAULT_DELEGATEE = _defaultDelegatee; | ||
| _transferOwnership(_owner); | ||
| } | ||
|
|
||
| /** | ||
| * @notice Sets the Merkle root for the airdrop. Can only be called by the owner. | ||
| * If allowMerkleRootUpdate is false, can only be called once. | ||
| * If ALLOW_MERKLE_ROOT_UPDATE;is false, can only be called once. | ||
| * When updating an existing merkle root, the contract must be paused to prevent front-running. | ||
| * When the merkle root is updated, the epoch is incremented, creating a new bitmap. | ||
| * @param _merkleRoot The Merkle root to set | ||
| */ | ||
| function setMerkleRoot(bytes32 _merkleRoot) external onlyOwner { | ||
| if (!allowMerkleRootUpdate && merkleRoot != bytes32(0)) { | ||
| if (!ALLOW_MERKLE_ROOT_UPDATE && merkleRoot != bytes32(0)) { | ||
| revert KarmaAirdrop__MerkleRootAlreadySet(); | ||
| } | ||
|
|
||
| // When updating an existing merkle root (not the first time), contract must be paused | ||
| if (allowMerkleRootUpdate && merkleRoot != bytes32(0) && !paused()) { | ||
| if (ALLOW_MERKLE_ROOT_UPDATE && merkleRoot != bytes32(0) && !paused()) { | ||
| revert KarmaAirdrop__MustBePausedToUpdate(); | ||
| } | ||
|
|
||
|
|
@@ -87,14 +89,15 @@ contract KarmaAirdrop is Ownable2Step, Pausable { | |
| uint256 claimedWordIndex = index / 256; | ||
| uint256 claimedBitIndex = index % 256; | ||
| uint256 claimedWord = claimedBitMap[epoch][claimedWordIndex]; | ||
| uint256 mask = (1 << claimedBitIndex); | ||
| uint256 mask = (BITMAP_MASK_BASE << claimedBitIndex); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is to avoid a warning that one might have swapped the two operands. |
||
| return claimedWord & mask == mask; | ||
| } | ||
|
|
||
| function _setClaimed(uint256 index) private { | ||
| uint256 claimedWordIndex = index / 256; | ||
| uint256 claimedBitIndex = index % 256; | ||
| claimedBitMap[epoch][claimedWordIndex] = claimedBitMap[epoch][claimedWordIndex] | (1 << claimedBitIndex); | ||
| claimedBitMap[epoch][claimedWordIndex] = | ||
| claimedBitMap[epoch][claimedWordIndex] | (BITMAP_MASK_BASE << claimedBitIndex); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -130,20 +133,21 @@ contract KarmaAirdrop is Ownable2Step, Pausable { | |
| } | ||
|
|
||
| // Verify the merkle proof. | ||
| /// forge-lint: disable-next-line(asm-keccak256) | ||
| bytes32 node = keccak256(abi.encodePacked(index, account, amount)); | ||
| if (!MerkleProof.verify(merkleProof, merkleRoot, node)) { | ||
| revert KarmaAirdrop__InvalidProof(); | ||
| } | ||
|
|
||
| // Mark it claimed and send the token. | ||
| _setClaimed(index); | ||
| if (!IERC20(token).transfer(account, amount)) { | ||
| if (!IERC20(TOKEN).transfer(account, amount)) { | ||
| revert KarmaAirdrop__TransferFailed(); | ||
| } | ||
|
|
||
| // If the account has no karma balance before this claim, delegate to the default delegatee | ||
| if (IERC20(token).balanceOf(account) == amount) { | ||
| IVotes(token).delegateBySig(defaultDelegatee, nonce, expiry, v, r, s); | ||
| if (IERC20(TOKEN).balanceOf(account) == amount) { | ||
| IVotes(TOKEN).delegateBySig(DEFAULT_DELEGATEE, nonce, expiry, v, r, s); | ||
| } | ||
|
|
||
| emit Claimed(index, account, amount); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasoning for the above:
__init()functions being one example).ERC20VotesUpgradeablebecause it violates many of the linting rules but is 99% library code we don't really maintain (we just changed one line in that contract)