Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chg: remove allowance checks. #36

Merged
merged 2 commits into from
Nov 24, 2021
Merged

chg: remove allowance checks. #36

merged 2 commits into from
Nov 24, 2021

Conversation

twygod
Copy link
Contributor

@twygod twygod commented Nov 23, 2021

No description provided.

@twygod twygod requested a review from maxweng November 23, 2021 07:49
@linear
Copy link

linear bot commented Nov 23, 2021

UNI-354 stake function in UserManager checks for allowance, which is also done in ERC20 transferFrom #24

code-423n4/2021-10-union-findings#24

Vulnerability details

Just before calling safeTransferFrom the stake function has a require statement which checks whether the allowance of the contract address is enough to transfer amount from msg.sender. The same require statement is already available in the OpenZeppelin implementation of transferFrom (and indirectly safeTransferFrom), which results in the require statement being redundant.

Impact

Calling stake is slightly more expensive on successful calls as there is an extra require statement to go through.

Proof of Concept

Require statement in stake function:
https://github.com/code-423n4/2021-10-union/blob/main/contracts/user/UserManager.sol#L619-L622

Require statement in OpenZeppelin ERC20 implementation of transferFrom:
https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/ERC20Upgradeable.sol#L163

@GeraldHost GeraldHost mentioned this pull request Nov 23, 2021
Merged
@maxweng maxweng merged commit 4b3a6e3 into develop Nov 24, 2021
@maxweng maxweng deleted the feature/uni-354-stake-function-in-usermanager-checks-for branch November 24, 2021 07:21
twygod added a commit that referenced this pull request Nov 24, 2021
@twygod
Merge branch 'develop' into feature/uni-371-usermanager-addmember-con…
8daea45 
…tains-redundant

* develop:
  chg: removing the reserveFactorMantissa_ >= 0. (#38)
  chg: remove allowance checks. (#36)
  chg: remove overflow and underflow checks. (#35)
  chg: Use short circuiting. (#32)
  chg: caching variables in loop. (#31)
  chg: caching multiple used variables in treasury. (#30)
  chg: Change UNION total supply to 1 Billion. (#34)
  chg: optimized registerMember. (#33)
maxweng added a commit that referenced this pull request Dec 16, 2021
@maxweng @twygod
@GeraldHost @kingjacob
Ready for the mainnet deployment (#75)
db1ab54 
* new: add deployment scripts and integration tests

* chg: update deploy script to check if the parameter has been set.

* new: deploy contract on kovan.

* new: add Infura key and test mnemonic to CI

* fix: fix typos

* chg: update deployed addresses

* chg: Determine whether to deploy compoundAdapter or aaveadapter according to the configuration.

* new: add contract upgradeability info in README

* chg: Determine whether to deploy compoundAdapter or aaveadapter according to the configuration.

* Feature/uni 276 resolve compiler warnings (#2)

* chg: resolve compiler warnings.

* new: add SPDX-License-Identifier: UNLICENSED.

* fix: fix solhint warnings

* chg: update tests

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* Feature/uni 173 create batchupdatetotalfrozen in (#3)

* chg: resolve compiler warnings.

* new: add SPDX-License-Identifier: UNLICENSED.

* fix: fix solhint warnings

* chg: update tests

* new: add batchUpdateTotalFrozen function.

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* Feature/uni 279 modify the deployment scripts and (#4)

* chg: call the initialization method of UUPS in the controller contract.

* chg: Update the deployment steps, modify the wrong parameters, update the judgment conditions of some method execution to avoid repeated calls.

* chg: deploy contract on kovan

* chg: update Kovan addresses

* Feature/uni 280 upgrade openzeppelincontracts to fix uups error (#6)

* chg: call the initialization method of UUPS in the controller contract.

* chg: Update the deployment steps, modify the wrong parameters, update the judgment conditions of some method execution to avoid repeated calls.

* chg: deploy contract on kovan

* chg: upgrade openzeppelin/contracts.!minor

* chg: disable contract size limit for mockup tests

* new: add compound adapter integration test.

* fix: Missing events for owner only functions that change critical parameters.

* chg: revert commit.!minor

* Feature/uni 315 zero address checks are missing (#10)

* chg: Change voting period from 1 week to 3 days.

* chg: add Zero-address checks.

* chg: add Zero-address checks.

* chg: Change voting period from 1 week to 3 days. (#9)

* chg: removal functions. (#23)

* chg: resolved TODOs. !minor (#24)

* fix: Change in interest rate can disable repay of loan. (#21)

* fix: Change in interest rate can disable repay of loan.

* chg: update InterestRatemodel test.

* chg: remove unuse function.!minor (#20)

* chg: break out of these loops earlier. (#25)

* fix: Rebalance will fail due to low precision of percentages. (#18)

* fix: Rebalance will fail due to low precision of percentages.

* chg: update test.

* chg: update epsilon.

* chg: Lock pragma compiler version. (#17)

* fix: Missing events for owner only functions that change critical parameters. (#16)

(cherry picked from commit d6cc02b)

* chg: halfdecay point to not be 0. (#12)

* chg: Remove CreditByMedian.sol and libraries that are only used by it. (#15)

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: UToken.sol should inherits and complies with IUToken.sol. (#14)

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* Enable 2-step admin change (#27)

* chg: enable 2-step admin change

* chg: add zero address guard

* chg: use getLastRepay() and getBorrowed() for consistency. (#26)

* new: add script to check deploy params. (#8)

* fix: borrow must accrueInterest first. (#13)

* fix: borrow must accrueInterest first.

* fix: remove repeat accrueInterest call.

* fix: Unbounded iteration in deleteMarket. (#22)

* fix: Unbounded iteration in deleteMarket.

* chg: revert when the address already exists use addUToken and addUserManager.

* chg: remove repeated require.

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* Feature/uni 334 comptroller rewards can be artificially (#19)

* chg: set min totalstake amount.

* chg: update test.

* chg: add test to compare the rewards of different stake amounts.

* chg: optimized registerMember. (#33)

* chg: optimized registerMember.

* chg: update test.

* chg: Change UNION total supply to 1 Billion. (#34)

* chg: Change UNION total supply to 1 Billion.

* chg: update test.!minor

* chg: caching multiple used variables in treasury. (#30)

* chg: caching multiple used variables in treasury.

(cherry picked from commit 925500e)

* chg: caching multiple used variables in treasury.

* chg: caching variables in loop. (#31)

* chg: Use short circuiting. (#32)

* chg: remove overflow and underflow checks. (#35)

* chg: remove allowance checks. (#36)

* chg: remove allowance checks.

* chg: update test.!minor

* chg: removing the reserveFactorMantissa_ >= 0. (#38)

* chg: UToken.sol _redeemFresh could be set private instead internal. (#40)

* [Feature] gas report deltas (#39)

* feat: gas report deltas

* feat: reverse colors

* chg: forking mainnet test only in integration test.

* feat: order files by time and fix gas delta calc

* fix: file ordering

* chg: update optimization run times to keep contracts under the size limit

* chg: run unit tests only to generate gas reports

* chg: add gas usage comparison task to github push action

* chg: move gas usage comparison task from github push to pull request action

Co-authored-by: twygod <twygod@gmail.com>
Co-authored-by: Max Weng <max.m.weng@gmail.com>

* fix: fixed typo

* chg: update gas report for dev branch

* chg: remove gas report for old commits

* chg: immutable variables. (#41)

* chg: Removing unnecessary initializations and branches. (#37)

* chg: Removing unnecessary initializations and branches.

* chg: optimized code.!minor

* Feature/uni 371 usermanager addmember contains redundant (#42)

* chg: remove redundant require check.

* chg: update test.!minor

* chg: usage of ternary made more readable. (#43)

* chg: usage of ternary made more readable.

* fix: Documentation tag is missing return parameter name.!minor

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: optimization  _repayBorrowFresh function. (#46)

* chg: remove unused imports. (#47)

* fix: fix test errors

* chg: update gas reports for dev branch

* feat: if statements to ternaries

* chg: Removing unnecessary initializations, returns, and reading of state variables. (#28)

* chg: add reentrancy guard to repayBorrowWithPermit() (#52)

* chg: addAdapter could add the new adapter as the last element to withdrawSeq. (#45)

* chg: change BORROW_RATE_MAX_MANTISSA to ~100% per year. (#50)

* chg: improve numeric precision and gas efficiency. (#53)

* chg: Cache array length in for loops. (#29)

* chg: Cache array length in for loops.

* chg: caching multiple used variables in treasury.

* fix: revert error commit.!minor

* chg: cache market on assetmanager.

* chg: fix compile errors

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* Feature/uni 382 add setter function for max stake in (#56)

* new: add setMaxStakeAmount function.

* chg: update test.!minor

* chg: update test.!minor

* chg: use custom errors

* fix: wrong judgment condition.

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* Long Revert Strings #17 (#48)

* Update UserManager.sol

* chg: shrink strings on uToken.sol

* fix: format user manager

* fix: update require message text in tests

* fix: revert max stake change

* fix: revert strings

* chg: shrink revert strings

* chg: shrink assetmanager revert strings

* chg: update assetmanager test.!minor

* chg: increase the waiting block.!minor

Co-authored-by: Gerald <me@jacobford.co.uk>
Co-authored-by: twygod <twygod@gmail.com>
Co-authored-by: Max Weng <max.m.weng@gmail.com>

* Feature/uni 379 combine utoken and uerc20 (#55)

* chg: Combine utoken and uerc20.

* chg: have UToken to use ERC20Permit directly

* chg: use enum show error message.

* chg: use custom error show error message.

* chg: add UDai contract.

* chg: rename getRemainingDebtCeiling()

* chg: rename custom errors

* chg: add test case to test borrow when credit limit is negative.

* chg: need to reset allowance for token compatibility

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: remove redundant continue statement. (#44)

* chg: Update script to setup timelock, admin and pause guardian's mult… (#51)

* chg: Update script to setup timelock, admin and pause guardian's multi-sig.

* chg: increase the waiting block.!minor

* chg: change deploy config to js format and remove local deploy artifacts.

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: Improve the sorting algorithm of SumOfTrust contract. (#11)

* chg: Improve the sorting algorithm of SumOfTrust contract.

* new: add test to test gas consumption of various sorting algorithms.

* chg: update sort test.!minor

* chg: update sort test.!minor

* chg: use odd even sort.

* chg: update sort test.

* chg: update sort test.

* chg: use bubble sort.

* chg: test insertion and inPlaceMerge sort results.

* chg: Change the mint cap from 4% to 2%. (#58)

* Feature/uni 387 the update script has tested the (#59)

* new: add maxStakeAmount check.

* chg: update script has tested the deployment process.

* chg: add deploy script test to CI

* chg: upgrade to node v14 for CI

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: upgrade openzeppelin version to 4.4 (#57)

* chg: update UnionToken `mintingAllowedAfter` on Kovan

* chg: deploy contract on kovan.

* Feature/uni 388 modify the permission setting script (#60)

* new: add maxStakeAmount check.

* chg: update script has tested the deployment process.

* chg: add deploy script test to CI

* chg: upgrade to node v14 for CI

* chg: modify the permission setting script

* fix: name error.!minor

* chg: annotate tests not used.!minor

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: update README

* Feature/uni 388 modify the permission setting script (#63)

* new: add maxStakeAmount check.

* chg: update script has tested the deployment process.

* chg: add deploy script test to CI

* chg: upgrade to node v14 for CI

* chg: modify the permission setting script

* fix: name error.!minor

* chg: annotate tests not used.!minor

* chg: update set auth script.

* chg: Add UnionToken whitelist address, modify admin to multi-signature address, modify unionToken distribution amount.

* chg: modify unionToken distribution amount.!minor

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* Fix slither tests (#66)

* chg: import IDai from the interface.

* chg: use SafeERC20 for the underlying token.

* fix: compiler warnings.

* chg: upgrade CI push task to use node v14

* chg: use capital case for custom errors, and remove unused ones.

* Feature/uni 383 add ability for timelock and admin to (#64)

* new: add claimRewards function on aave and compound adapter.

* chg: update aave and compound adapter.

* chg: update test.!minor

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: set admin key and pause gaurdian. (#65)

* chg: set admin key and pause gaurdian.

* chg: update pause guardian address

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* new: add main network deploy params. (#61)

* new: add main network deploy params.

* chg: set admin key and pause gaurdian.

* chg: set half way point in the half-decay function to 500,000

* chg: set max stake amount to 5000

* chg: add adapters in the order of pure token, compound, and aave

* chg: use CompoundMock for local tests

* chg: add AaveAdapter after deployment checks.

* chg: update initial deployment params.

* chg: fix failed tests

* chg: add mainnet config

* new: add new task for listing accounts for current network settings.

* chg: set treasury's dripStart time in config so it can be checked after the deployment

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: unionToken modify permissions are divided into admin and minter. (#67)

* chg: unionToken modify permissions are divided into admin and minter.

* chg: unionToken modify permissions and test.

* chg: update setAuth script.

* chg: Update Governance with GovernorSettings. (#68)

* chg: Update Governance with GovernorSettings.

* chg: change initial proposal threshold from 50k to 1M.

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* new: add treasury integration test. (#69)

* new: add treasury integration test.

* chg: update treasury test.

* fix: Modify deploy to fix initializer: false invalid caused by lib upgrade.

* Feature/uni 421 add minter role to unionsol (#70)

* chg: unionToken modify permissions are divided into admin and minter.

* chg: unionToken modify permissions and test.

* chg: update setAuth script.

* fix: Modify deploy to fix initializer: false invalid caused by lib upgrade.

* chg: Update uniontoken permission control and testing.

* fix: merge error.!minor

* chg: Update uniontoken permission control and testing.

* chg: updated the deploy config for final kovan test.

* chg: add withdraw sequence config for AssetManager.

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: update kovan deployments.

* fix: origination fee config.

* chg: update treasury contract dripRate params. (#71)

* chg: update treasury contract dripRate params.

* chg: add comments. !minor

Co-authored-by: Max Weng <max.m.weng@gmail.com>

* chg: set timelock permissions to multi-signature addresses. (#72)

* chg: set timelock permissions to multi-signature addresses.

* chg: set timelock admin permissions to multi-signature addresses.

* chg: add timelock admin role check.

* Update proposal threshold to 10M. (#74)

Co-authored-by: twygod <twygod@gmail.com>
Co-authored-by: Gerald Host <me@jacobford.co.uk>
Co-authored-by: Jacob Shiach <kingjacob@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeraldHost GeraldHost GeraldHost approved these changes

@maxweng maxweng maxweng approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@twygod @maxweng @GeraldHost