From 830a7fa0636b9c9691538d51888240baaa1a7cd7 Mon Sep 17 00:00:00 2001 From: has5aan <50018215+has5aan@users.noreply.github.com> Date: Wed, 3 Jul 2024 12:35:03 +0200 Subject: [PATCH] Resolves warnings from slither (#190) * Modifies L2Reward and L2LockingPosition to supress or resolve slither warnings disables unchecked-transfer rule for all calls to IL2LiskToken.transferFrom in L2Reward disables unused-return rule for all calls to IL2LiskToken.approve in L2Reward disables unused-return rule for all calls to Math.trySub in L2Reward disables reentrancy-no-eth for L2Reward._delePosition disables reentrancy-no-eth for L2Reward._initiateFastUnlock resolves warnings in L2Reward.createPosition disables uninitialized-state for L2LockingPosition.lockingPosition state variable * Adds additional comments to slither warning supression hints --- .github/workflows/pr.yaml | 2 +- src/L2/L2LockingPosition.sol | 2 ++ src/L2/L2Reward.sol | 31 +++++++++++++++++++++++++------ 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index 7e537ef3..e5af8195 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -58,7 +58,7 @@ jobs: with: solc-version: 0.8.23 sarif: results.sarif - fail-on: none + fail-on: medium slither-args: --exclude-dependencies --exclude-low --exclude-informational --filter-paths "Ed25519.sol" id: slither diff --git a/src/L2/L2LockingPosition.sol b/src/L2/L2LockingPosition.sol index e849a2dd..24780ba8 100644 --- a/src/L2/L2LockingPosition.sol +++ b/src/L2/L2LockingPosition.sol @@ -21,6 +21,8 @@ contract L2LockingPosition is Initializable, Ownable2StepUpgradeable, UUPSUpgrad uint256 private nextId; /// @notice Mapping of locking position ID to LockingPosition entity. + // lockingPositions mapping can be modified only against user actions. + // slither-disable-next-line uninitialized-state mapping(uint256 => IL2LockingPosition.LockingPosition) public lockingPositions; /// @notice Address of the Staking contract. diff --git a/src/L2/L2Reward.sol b/src/L2/L2Reward.sol index cd0eb809..1553eb4a 100644 --- a/src/L2/L2Reward.sol +++ b/src/L2/L2Reward.sol @@ -172,12 +172,7 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS function createPosition(uint256 amount, uint256 duration) public virtual returns (uint256) { updateGlobalState(); - // create a new position - IL2LiskToken(l2TokenContract).transferFrom(msg.sender, address(this), amount); - IL2LiskToken(l2TokenContract).approve(stakingContract, amount); - uint256 id = IL2Staking(stakingContract).lockAmount(msg.sender, amount, duration); uint256 today = todayDay(); - lastClaimDate[id] = today; // update total weight and amount totalWeight += amount * (duration + OFFSET); @@ -185,6 +180,16 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS dailyUnlockedAmounts[today + duration] += amount; pendingUnlockAmount += amount; + // create a new position + // IL2LiskToken.transferFrom always returns true and reverts in case of error + // slither-disable-next-line unchecked-transfer + IL2LiskToken(l2TokenContract).transferFrom(msg.sender, address(this), amount); + // IL2LiskToken.approve always returns true and reverts in case of error + // slither-disable-next-line unused-return + IL2LiskToken(l2TokenContract).approve(stakingContract, amount); + uint256 id = IL2Staking(stakingContract).lockAmount(msg.sender, amount, duration); + lastClaimDate[id] = today; + emit LockingPositionCreated(id); return id; @@ -212,6 +217,8 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS _claimReward(lockID); IL2Staking(stakingContract).unlock(lockID); + // lockID can be deleted only when rewards are claimed and the position is unlocked by L2Staking contract. + // slither-disable-next-line reentrancy-no-eth delete lastClaimDate[lockID]; emit LockingPositionDeleted(lockID); @@ -236,6 +243,7 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS /// @param lockID The ID of the locking position. function _initiateFastUnlock(uint256 lockID) internal virtual { // claim rewards and inform staking contract + // slither-disable-next-line reentrancy-no-eth _claimReward(lockID); IL2LockingPosition.LockingPosition memory lockingPositionBeforeInitiatingFastUnlock = IL2LockingPosition(lockingPositionContract).getLockingPosition(lockID); @@ -285,6 +293,9 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS if (lockingPosition.pausedLockingDuration == 0) { // If rewards are claimed against an expired position that was already claimed after expiry period, // rewardableDuration is zero + // If lastClaimDate for lockID is greater than expiry date, Math.trySub sets rewardableDuration to zero + // and the overflow flag can be ignored. + // slither-disable-next-line unused-return (, rewardableDuration) = Math.trySub(lockingPosition.expDate, lastClaimDate[lockID]); lastRewardDay = Math.min(lockingPosition.expDate, today); @@ -293,7 +304,7 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS lastRewardDay = today; } - uint256 reward; + uint256 reward = 0; if (rewardableDuration > 0) { weight = lockingPosition.amount * (rewardableDuration + OFFSET); @@ -348,6 +359,8 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS lastClaimDate[lockID] = today; if (reward != 0) { + // IL2LiskToken.transfer always returns true and reverts in case of error + // slither-disable-next-line unchecked-transfer IL2LiskToken(l2TokenContract).transfer(msg.sender, reward); } } @@ -375,7 +388,11 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS function _increaseLockingAmount(uint256 lockID, uint256 amountIncrease) internal virtual { // claim rewards and update staking contract _claimReward(lockID); + // IL2LiskToken.transferFrom always returns true and reverts in case of error + // slither-disable-next-line unchecked-transfer IL2LiskToken(l2TokenContract).transferFrom(msg.sender, address(this), amountIncrease); + // IL2LiskToken.approve always returns true and reverts in case of error + // slither-disable-next-line unused-return IL2LiskToken(l2TokenContract).approve(stakingContract, amountIncrease); IL2Staking(stakingContract).increaseLockingAmount(lockID, amountIncrease); @@ -556,6 +573,8 @@ contract L2Reward is Initializable, Ownable2StepUpgradeable, UUPSUpgradeable, IS require(duration > 0, "L2Reward: Funding duration should be greater than zero"); require(delay > 0, "L2Reward: Funding should start from next day or later"); + // IL2LiskToken.transferFrom always returns true and reverts in case of error + // slither-disable-next-line unchecked-transfer IL2LiskToken(l2TokenContract).transferFrom(msg.sender, address(this), amount); _addRewards(amount, duration, delay);