Fix slither warnings #99

maxweng · 2022-05-08T19:02:38Z

AssetManager.rebalance(address,uint256[]) (contracts/asset/AssetManager.sol#396-441) uses a dangerous strict equality:
- amountToDeposit == 0 (contracts/asset/AssetManager.sol#423)
CompoundAdapter.deposit(address) (contracts/asset/CompoundAdapter.sol#97-108) uses a dangerous strict equality:
- require(bool,string)(result == 0,Error minting the cToken) (contracts/asset/CompoundAdapter.sol#107)
UToken.repayBorrowFresh(address,address,uint256) (contracts/market/UToken.sol#437-493) uses a dangerous strict equality:
- repayAmount == 0 (contracts/market/UToken.sol#452)
UToken.exchangeRateStored() (contracts/market/UToken.sol#348-351) uses a dangerous strict equality:
- totalSupply == 0 (contracts/market/UToken.sol#350)
Comptroller._calculateRewards(address,address,uint256,uint256,uint256,uint256,uint256) (contracts/token/Comptroller.sol#245-268) uses a dangerous strict equality:
- userStaked == 0 || totalStaked == 0 || startInflationIndex == 0 || pastBlocks == 0 (contracts/token/Comptroller.sol#257)
Comptroller.getInflationIndexNew(uint256,uint256) (contracts/token/Comptroller.sol#233-243) uses a dangerous strict equality:
- totalStaked == 0 (contracts/token/Comptroller.sol#234)
Comptroller.getInflationIndexNew(uint256,uint256) (contracts/token/Comptroller.sol#233-243) uses a dangerous strict equality:
- blockDelta == 0 (contracts/token/Comptroller.sol#238)
Comptroller.getRewardsMultiplier(uint256,uint256,uint256,bool) (contracts/token/Comptroller.sol#322-342) uses a dangerous strict equality:
- userStaked == 0 || totalFrozen >= lockedStake || totalFrozen >= userStaked (contracts/token/Comptroller.sol#329)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-strict-equalities

Reentrancy in UToken._repayBorrowFresh(address,address,uint256) (contracts/market/UToken.sol#437-493):
External calls:
- IUserManager(userManager).updateTotalFrozen(borrower,false) (contracts/market/UToken.sol#461)
- IUserManager(userManager).repayLoanOverdue(borrower,underlying,getLastRepay(borrower)) (contracts/market/UToken.sol#462)
State variables written after the call(s):
- accountBorrows[borrower].principal = borrowedAmount - repayAmount (contracts/market/UToken.sol#464)
- accountBorrows[borrower].interest = 0 (contracts/market/UToken.sol#465)
- accountBorrows[borrower].lastRepay = 0 (contracts/market/UToken.sol#469)
- accountBorrows[borrower].lastRepay = getBlockNumber() (contracts/market/UToken.sol#471)
- accountBorrows[borrower].interestIndex = borrowIndex (contracts/market/UToken.sol#483)
- totalBorrows -= repayAmount (contracts/market/UToken.sol#484)
Reentrancy in UserManager.batchUpdateTotalFrozen(address[],bool[]) (contracts/user/UserManager.sol#893-906):
External calls:
- comptroller.updateTotalStaked(stakingToken,effectiveTotalStaked) (contracts/user/UserManager.sol#902)
State variables written after the call(s):
- _updateTotalFrozen(accounts[i],isOverdues[i]) (contracts/user/UserManager.sol#904)
- totalFrozen += amount (contracts/user/UserManager.sol#922)
- totalFrozen = totalFrozen + amount - memberFrozen[account] (contracts/user/UserManager.sol#925)
- totalFrozen -= memberFrozen[account] (contracts/user/UserManager.sol#933)
- totalFrozen = 0 (contracts/user/UserManager.sol#935)
Reentrancy in UToken.borrow(uint256) (contracts/market/UToken.sol#383-419):
External calls:
- IUserManager(userManager).updateLockedData(msg.sender,newPrincipal - oldPrincipal,true) (contracts/market/UToken.sol#409)
State variables written after the call(s):
- accountBorrows[msg.sender].interest = accountBorrowsNew - newPrincipal (contracts/market/UToken.sol#410)
- accountBorrows[msg.sender].interestIndex = borrowIndex (contracts/market/UToken.sol#411)
- totalBorrows = totalBorrowsNew (contracts/market/UToken.sol#412)
Reentrancy in UserManager.debtWriteOff(address,uint256) (contracts/user/UserManager.sol#843-874):
External calls:
- comptroller.withdrawRewards(msg.sender,stakingToken) (contracts/user/UserManager.sol#854)
State variables written after the call(s):
- memberFrozen[borrower] -= amount (contracts/user/UserManager.sol#862)
- memberFrozen[borrower] = 0 (contracts/user/UserManager.sol#864)
- members[msg.sender].creditLine.lockedAmount[borrower] = lockedAmount - amount (contracts/user/UserManager.sol#866)
- members[msg.sender].creditLine.borrowers[borrower] = newTrustAmount (contracts/user/UserManager.sol#869)
- members[borrower].creditLine.stakers[msg.sender] = newTrustAmount (contracts/user/UserManager.sol#870)
- totalFrozen -= amount (contracts/user/UserManager.sol#860)
- totalStaked -= amount (contracts/user/UserManager.sol#859)
Reentrancy in UserManager.registerMember(address) (contracts/user/UserManager.sol#649-669):
External calls:
- effectiveStakerNumber < creditLimitModel.effectiveNumber() (contracts/user/UserManager.sol#662)
State variables written after the call(s):
- members[newMember].isMember = true (contracts/user/UserManager.sol#664)
Reentrancy in UserManagerArb.registerMember(address) (contracts/user/UserManagerArb.sol#13-33):
External calls:
- effectiveStakerNumber < creditLimitModel.effectiveNumber() (contracts/user/UserManagerArb.sol#26)
State variables written after the call(s):
- members[newMember].isMember = true (contracts/user/UserManagerArb.sol#28)
Reentrancy in UserManager.unstake(uint256) (contracts/user/UserManager.sol#791-808):
External calls:
- comptroller.withdrawRewards(msg.sender,stakingToken) (contracts/user/UserManager.sol#797)
State variables written after the call(s):
- stakers[msg.sender] = stakingAmount - amount (contracts/user/UserManager.sol#799)
Reentrancy in UserManager.updateTotalFrozen(address,bool) (contracts/user/UserManager.sol#881-886):
External calls:
- comptroller.updateTotalStaked(stakingToken,effectiveTotalStaked) (contracts/user/UserManager.sol#884)
State variables written after the call(s):
- _updateTotalFrozen(account,isOverdue) (contracts/user/UserManager.sol#885)
- totalFrozen += amount (contracts/user/UserManager.sol#922)
- totalFrozen = totalFrozen + amount - memberFrozen[account] (contracts/user/UserManager.sol#925)
- totalFrozen -= memberFrozen[account] (contracts/user/UserManager.sol#933)
- totalFrozen = 0 (contracts/user/UserManager.sol#935)
Reentrancy in AssetManager.withdraw(address,address,uint256) (contracts/asset/AssetManager.sol#217-258):
External calls:
- IERC20Upgradeable(token).safeTransfer(account,withdrawAmount) (contracts/asset/AssetManager.sol#231)
- supply = moneyMarket.getSupply(token) (contracts/asset/AssetManager.sol#241)
- moneyMarket.withdraw(token,account,withdrawAmount_scope_0) (contracts/asset/AssetManager.sol#246)
State variables written after the call(s):
- balances[msg.sender][token] = balances[msg.sender][token] - amount + remaining (contracts/asset/AssetManager.sol#251)
- totalPrincipal[token] = totalPrincipal[token] - amount + remaining (contracts/asset/AssetManager.sol#252)
Reentrancy in Comptroller.withdrawRewards(address,address) (contracts/token/Comptroller.sol#92-120):
External calls:
- unionToken.safeTransfer(sender,amount) (contracts/token/Comptroller.sol#109)
State variables written after the call(s):
- users[sender][token].accrued = 0 (contracts/token/Comptroller.sol#110)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-1

PureTokenAdapter._supportsToken(address) (contracts/asset/PureTokenAdapter.sol#92-94) contains a tautology or contradiction:
- tokenAddress != address(0) && IERC20Upgradeable(tokenAddress).balanceOf(address(this)) >= 0 (contracts/asset/PureTokenAdapter.sol#93)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#tautology-or-contradiction

AssetManager.removeAdapter(address).index (contracts/asset/AssetManager.sol#339) is a local variable never initialized
UserManager.getCreditLimit(address).trustInfo (contracts/user/UserManager.sol#427) is a local variable never initialized
AssetManager.removeToken(address).index (contracts/asset/AssetManager.sol#284) is a local variable never initialized
Treasury.addSchedule(uint256,uint256,address,uint256).schedule (contracts/treasury/Treasury.sol#96) is a local variable never initialized
UserManager.updateLockedData(address,uint256,bool).trustInfo (contracts/user/UserManager.sol#681) is a local variable never initialized
Comptroller.calculateRewardsByBlocks(address,address,uint256).userManagerData (contracts/token/Comptroller.sol#136) is a local variable never initialized
UserManager.updateTrust(address,uint256).trustInfo (contracts/user/UserManager.sol#526) is a local variable never initialized
UserManager.getTotalFrozenAmount(address).trustInfo (contracts/user/UserManager.sol#400) is a local variable never initialized
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-local-variables

MarketRegistry.addUToken(address,address) (contracts/market/MarketRegistry.sol#57-62) ignores return value by uTokenList.add(uToken) (contracts/market/MarketRegistry.sol#59)
MarketRegistry.addUserManager(address,address) (contracts/market/MarketRegistry.sol#64-72) ignores return value by userManagerList.add(userManager) (contracts/market/MarketRegistry.sol#69)
MarketRegistry.deleteMarket(address) (contracts/market/MarketRegistry.sol#74-79) ignores return value by uTokenList.remove(tokens[token].uToken) (contracts/market/MarketRegistry.sol#75)
MarketRegistry.deleteMarket(address) (contracts/market/MarketRegistry.sol#74-79) ignores return value by userManagerList.remove(tokens[token].userManager) (contracts/market/MarketRegistry.sol#76)
UToken._repayBorrowFresh(address,address,uint256) (contracts/market/UToken.sol#437-493) ignores return value by IUserManager(userManager).repayLoanOverdue(borrower,underlying,getLastRepay(borrower)) (contracts/market/UToken.sol#462)
ArbUnionWrapper.registerTokenOnL2(address,uint256,uint256,uint256,uint256) (contracts/token/ArbUnionWrapper.sol#66-92) ignores return value by IGatewayRouter(router).setGateway{value: gas2}(gateway,maxGas,gasPriceBid,maxSubmissionCostForRouter) (contracts/token/ArbUnionWrapper.sol#88)
ArbConnector.bridge(uint256,uint256,uint256) (contracts/treasury/ArbConnector.sol#36-58) ignores return value by IGatewayRouter(gatewayRouter).outboundTransfer{value: msg.value}(address(arbUnionWrapper),target,transferAmount,maxGas,gasPriceBid,data) (contracts/treasury/ArbConnector.sol#47-54)
ArbConnector.claimTokens(address) (contracts/treasury/ArbConnector.sol#60-70) ignores return value by arbUnionWrapper.unwrap(wBalance) (contracts/treasury/ArbConnector.sol#64)
UserManager.stake(uint256) (contracts/user/UserManager.sol#726-744) ignores return value by comptroller.withdrawRewards(msg.sender,stakingToken) (contracts/user/UserManager.sol#729)
UserManager.unstake(uint256) (contracts/user/UserManager.sol#791-808) ignores return value by comptroller.withdrawRewards(msg.sender,stakingToken) (contracts/user/UserManager.sol#797)
UserManager.withdrawRewards() (contracts/user/UserManager.sol#810-812) ignores return value by comptroller.withdrawRewards(msg.sender,stakingToken) (contracts/user/UserManager.sol#811)
UserManager.debtWriteOff(address,uint256) (contracts/user/UserManager.sol#843-874) ignores return value by comptroller.withdrawRewards(msg.sender,stakingToken) (contracts/user/UserManager.sol#854)
UserManager.updateTotalFrozen(address,bool) (contracts/user/UserManager.sol#881-886) ignores return value by comptroller.updateTotalStaked(stakingToken,effectiveTotalStaked) (contracts/user/UserManager.sol#884)
UserManager.batchUpdateTotalFrozen(address[],bool[]) (contracts/user/UserManager.sol#893-906) ignores return value by comptroller.updateTotalStaked(stakingToken,effectiveTotalStaked) (contracts/user/UserManager.sol#902)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return
. analyzed (121 contracts with 44 detectors), 41 result(s) found

Merge Arbitrum

* docs: basic user manager comments * fix: lint warning on comment Co-authored-by: Min Weng (Max) <max.m.weng@gmail.com>

* chg: add code coverage to CI push action. * chg: update CI push action.

…tests.

linear · 2022-05-08T19:02:40Z

UNI-718 Fix Slither warnings

codecov · 2022-05-08T19:10:10Z

Codecov Report

Merging #99 (88a8332) into master (208206d) will increase coverage by 0.59%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   89.90%   90.49%   +0.59%     
==========================================
  Files          24       24              
  Lines        1525     1526       +1     
  Branches      250      250              
==========================================
+ Hits         1371     1381      +10     
+ Misses        154      145       -9

Impacted Files	Coverage Δ
contracts/asset/CompoundAdapter.sol	`97.50% <ø> (ø)`
contracts/market/MarketRegistry.sol	`100.00% <ø> (ø)`
contracts/token/ArbUnionWrapper.sol	`0.00% <ø> (ø)`
contracts/treasury/ArbConnector.sol	`0.00% <ø> (ø)`
contracts/treasury/Treasury.sol	`98.36% <ø> (ø)`
contracts/user/UserManagerArb.sol	`87.50% <ø> (ø)`
contracts/asset/AssetManager.sol	`96.58% <100.00%> (ø)`
contracts/asset/PureTokenAdapter.sol	`87.80% <100.00%> (ø)`
contracts/market/UToken.sol	`94.86% <100.00%> (ø)`
contracts/token/Comptroller.sol	`93.70% <100.00%> (+4.72%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 208206d...88a8332. Read the comment docs.

kingjacob · 2022-05-16T16:17:57Z

contracts/market/UToken.sol

@@ -406,12 +410,13 @@ contract UToken is IUToken, Controller, ERC20PermitUpgradeable, ReentrancyGuardU

        accountBorrows[msg.sender].principal += amount + fee;
        uint256 newPrincipal = getBorrowed(msg.sender);
-        IUserManager(userManager).updateLockedData(msg.sender, newPrincipal - oldPrincipal, true);


Why move this?

This is to follow the checks-effects-interactions pattern.

kingjacob · 2022-05-16T16:18:42Z

contracts/token/Comptroller.sol

@@ -106,8 +106,8 @@ contract Comptroller is Controller, IComptroller {
        users[sender][token].updatedBlock = block.number;
        users[sender][token].inflationIndex = gInflationIndex;
        if (unionToken.balanceOf(address(this)) >= amount && amount > 0) {
-            unionToken.safeTransfer(sender, amount);


why move this?

This is to follow the checks-effects-interactions pattern.

kingjacob · 2022-05-16T16:21:27Z

contracts/user/UserManager.sol

        address[] memory stakerAddresses = getStakerAddresses(account);
-        uint256 addressesLength;


Does this change need to be onchain?

This is an optimization to cache the number of addresses used in the loop.

kingjacob · 2022-05-16T16:21:49Z

contracts/user/UserManager.sol

     */
    function repayLoanOverdue(
        address account,
        address token,
        uint256 lastRepay
-    ) external override whenNotPaused onlyMarketOrAdmin {


what's the reason for this counter?

It's supposed to be called by UToken

kingjacob · 2022-05-16T16:22:11Z

contracts/user/UserManager.sol

        comptroller.updateTotalStaked(stakingToken, effectiveTotalStaked);
-        _updateTotalFrozen(account, isOverdue);


why was this moved?

This is to follow the checks-effects-interactions pattern.

maxweng and others added 12 commits March 4, 2022 11:25

Merge pull request #94 from unioncredit/develop

c68f939

Merge Arbitrum

docs: basic user manager comments (#77)

9b56085

* docs: basic user manager comments * fix: lint warning on comment Co-authored-by: Min Weng (Max) <max.m.weng@gmail.com>

chg: update Governor's address on mainnet.

b3aecf9

Fix the code coverage display issues

f3fbb6f

Fix test coverage reporting bugs (#96)

048fdea

* chg: add code coverage to CI push action. * chg: update CI push action.

chg: update code coverage badge

208206d

fix: repayLoanOverdue func addressesLength is not assigned.

489cf2a

fix: addressesLength var not init in repayLoanOverdue() and add more …

a15d2cd

…tests.

chg: remove unnecessary code.

6e62004

fix: failed tests.

bd0c3b8

chg: remove obsoleted tests.

aebda77

chg: enable slither medium level warnings.

66766dc

twygod and others added 7 commits May 9, 2022 15:19

fix: reentrancy warning.

d60ae3c

fix: strict equality warning.

b8928ec

fix: reentrancy warning.

fc90d73

fix: uninitialized local vars warnings.

4a5ccec

fix: redundant checks.

f7ad303

fix: unused return.

e5826fb

chg: update to pure functions.

88a8332

maxweng requested review from twygod, GeraldHost and kingjacob May 13, 2022 12:45

twygod approved these changes May 16, 2022

View reviewed changes

kingjacob reviewed May 16, 2022

View reviewed changes

maxweng changed the base branch from master to develop May 23, 2022 13:09

maxweng merged commit 7d2b9ac into develop May 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix slither warnings #99

Fix slither warnings #99

maxweng commented May 8, 2022 •

edited

Loading

linear bot commented May 8, 2022

codecov bot commented May 8, 2022 •

edited

Loading

kingjacob May 16, 2022

maxweng May 17, 2022

kingjacob May 16, 2022

maxweng May 17, 2022

kingjacob May 16, 2022

maxweng May 17, 2022

kingjacob May 16, 2022

maxweng May 17, 2022

kingjacob May 16, 2022

maxweng May 17, 2022

		address[] memory stakerAddresses = getStakerAddresses(account);
		uint256 addressesLength;

		comptroller.updateTotalStaked(stakingToken, effectiveTotalStaked);
		_updateTotalFrozen(account, isOverdue);

Fix slither warnings #99

Fix slither warnings #99

Conversation

maxweng commented May 8, 2022 • edited Loading

linear bot commented May 8, 2022

codecov bot commented May 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxweng commented May 8, 2022 •

edited

Loading

codecov bot commented May 8, 2022 •

edited

Loading