From 51da2a7c5e9fc5eb4b601397f2a486ad3ecd8e07 Mon Sep 17 00:00:00 2001 From: Pavel Rubin Date: Fri, 13 Nov 2020 14:11:24 +0300 Subject: [PATCH 1/7] Add reentrancy guard and check deposited tokens are registered --- contracts/modules/savings/SavingsModule.sol | 54 ++++++++++++++++++--- 1 file changed, 46 insertions(+), 8 deletions(-) diff --git a/contracts/modules/savings/SavingsModule.sol b/contracts/modules/savings/SavingsModule.sol index 1427164..3482525 100644 --- a/contracts/modules/savings/SavingsModule.sol +++ b/contracts/modules/savings/SavingsModule.sol @@ -58,12 +58,33 @@ contract SavingsModule is Module, AccessChecker, RewardDistributions, CapperRole mapping(address=>uint256) public protocolCap; bool public vipUserEnabled; // Enable VIP user (overrides protocol cap) + uint256 private _guardCounter; // See OpenZeppelin ReentrancyGuard. Copied here to allow upgrade of already deployed contracts + function initialize(address _pool) public initializer { Module.initialize(_pool); CapperRole.initialize(_msgSender()); + + _guardCounter = 1; // See OpenZeppelin ReentrancyGuard. + } + + function upgradeGuardCounter() public onlyOwner { + require(_guardCounter == 0, "Already upgraded"); + _guardCounter = 1; + } + + /** + * @dev Prevents a contract from calling itself, directly or indirectly. + * See OpenZeppelin ReentrancyGuard + */ + modifier nonReentrant() { + _guardCounter += 1; + uint256 localCounter = _guardCounter; + _; + require(localCounter == _guardCounter, "ReentrancyGuard: reentrant call"); } + function setUserCapEnabled(bool _userCapEnabled) public onlyCapper { userCapEnabled = _userCapEnabled; emit UserCapEnabledChange(userCapEnabled); @@ -198,18 +219,20 @@ contract SavingsModule is Module, AccessChecker, RewardDistributions, CapperRole * @param _tokens Array of tokens to deposit * @param _dnAmounts Array of amounts (denormalized to token decimals) */ - function deposit(address[] memory _protocols, address[] memory _tokens, uint256[] memory _dnAmounts) - public operationAllowed(IAccessModule.Operation.Deposit) + function deposit(address[] calldata _protocols, address[] calldata _tokens, uint256[] calldata _dnAmounts) + external nonReentrant operationAllowed(IAccessModule.Operation.Deposit) returns(uint256[] memory) { require(_protocols.length == _tokens.length && _tokens.length == _dnAmounts.length, "SavingsModule: size of arrays does not match"); + require(isAllTokensRegistered(_tokens), "SavingsModule: unsupported token"); + uint256[] memory ptAmounts = new uint256[](_protocols.length); for (uint256 i=0; i < _protocols.length; i++) { address[] memory tkns = new address[](1); tkns[0] = _tokens[i]; uint256[] memory amnts = new uint256[](1); amnts[0] = _dnAmounts[i]; - ptAmounts[i] = deposit(_protocols[i], tkns, amnts); + ptAmounts[i] = _deposit(_protocols[i], tkns, amnts); } return ptAmounts; } @@ -220,8 +243,15 @@ contract SavingsModule is Module, AccessChecker, RewardDistributions, CapperRole * @param _tokens Array of tokens to deposit * @param _dnAmounts Array of amounts (denormalized to token decimals) */ - function deposit(address _protocol, address[] memory _tokens, uint256[] memory _dnAmounts) - public operationAllowed(IAccessModule.Operation.Deposit) + function deposit(address _protocol, address[] calldata _tokens, uint256[] calldata _dnAmounts) + external nonReentrant operationAllowed(IAccessModule.Operation.Deposit) + returns(uint256) { + require(isAllTokensRegistered(_tokens), "SavingsModule: unsupported token"); + _deposit(_protocol, _tokens, _dnAmounts); + } + + function _deposit(address _protocol, address[] memory _tokens, uint256[] memory _dnAmounts) + internal returns(uint256) { //distributeRewardIfRequired(_protocol); @@ -293,7 +323,7 @@ contract SavingsModule is Module, AccessChecker, RewardDistributions, CapperRole * @return Amount of PoolToken burned from user */ function withdrawAll(address _protocol, uint256 nAmount) - public operationAllowed(IAccessModule.Operation.Withdraw) + external nonReentrant operationAllowed(IAccessModule.Operation.Withdraw) returns(uint256) { //distributeRewardIfRequired(_protocol); @@ -341,8 +371,9 @@ contract SavingsModule is Module, AccessChecker, RewardDistributions, CapperRole * @return Amount of PoolToken burned from user */ function withdraw(address _protocol, address token, uint256 dnAmount, uint256 maxNAmount) - public operationAllowed(IAccessModule.Operation.Withdraw) + external nonReentrant operationAllowed(IAccessModule.Operation.Withdraw) returns(uint256){ + require(isTokenRegistered(token), "SavingsModule: unsupported token"); //distributeRewardIfRequired(_protocol); uint256 nAmount = normalizeTokenAmount(token, dnAmount); @@ -388,7 +419,7 @@ contract SavingsModule is Module, AccessChecker, RewardDistributions, CapperRole /** * @notice Distributes yield. May be called by bot, if there was no deposits/withdrawals */ - function distributeYield() public { + function distributeYield() external { for(uint256 i=0; i Date: Fri, 13 Nov 2020 15:42:17 +0300 Subject: [PATCH 2/7] Add whitelist check for intermediate senders (which can only be contracts). --- contracts/modules/access/AccessModule.sol | 31 +++++++++++++---------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/contracts/modules/access/AccessModule.sol b/contracts/modules/access/AccessModule.sol index 702e2d3..ff25e8c 100644 --- a/contracts/modules/access/AccessModule.sol +++ b/contracts/modules/access/AccessModule.sol @@ -6,35 +6,40 @@ import "../../common/Module.sol"; import "../../interfaces/access/IAccessModule.sol"; contract AccessModule is Module, IAccessModule, Pausable, WhitelistedRole { - event WhitelistEnabled(); - event WhitelistDisabled(); + event WhitelistForAllStatusChange(bool enabled); + event WhitelistForIntermediateSendersStatusChange(bool enabled); - bool public whitelistEnabled; + bool public whitelistEnabledForAll; + bool public whitelistEnabledForIntermediateSenders; function initialize(address _pool) public initializer { Module.initialize(_pool); Pausable.initialize(_msgSender()); WhitelistedRole.initialize(_msgSender()); - // enableWhitelist(); //whitelist is disabled by default for testnet, will be enabled by default for mainnet } - function enableWhitelist() public onlyWhitelistAdmin { - whitelistEnabled = true; - emit WhitelistEnabled(); + function setWhitelistForAll(bool enabled) public onlyWhitelistAdmin { + whitelistEnabledForAll = enabled; + emit WhitelistForAllStatusChange(enabled); } - function disableWhitelist() public onlyWhitelistAdmin { - whitelistEnabled = false; - emit WhitelistDisabled(); + function setWhitelistForIntermediateSenders(bool enabled) public onlyWhitelistAdmin { + whitelistEnabledForIntermediateSenders = enabled; + emit WhitelistForIntermediateSendersStatusChange(enabled); } function isOperationAllowed(Operation operation, address sender) public view returns(bool) { (operation); //noop to prevent compiler warning if (paused()) return false; - if (!whitelistEnabled) { - return true; - } else { + if (whitelistEnabledForAll) { + return isWhitelisted(sender); + } else if( + whitelistEnabledForIntermediateSenders && + tx.origin != sender + ){ return isWhitelisted(sender); + } else { + return true; } } } From e8e264f568c8ab9dc9f500c695f40df24d347660 Mon Sep 17 00:00:00 2001 From: Pavel Rubin Date: Fri, 13 Nov 2020 17:03:44 +0300 Subject: [PATCH 3/7] Add check for gas left --- contracts/modules/access/AccessChecker.sol | 4 ++++ contracts/modules/access/AccessModule.sol | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/contracts/modules/access/AccessChecker.sol b/contracts/modules/access/AccessChecker.sol index 92e3d14..90c5200 100644 --- a/contracts/modules/access/AccessChecker.sol +++ b/contracts/modules/access/AccessChecker.sol @@ -9,5 +9,9 @@ contract AccessChecker is Module { IAccessModule am = IAccessModule(getModuleAddress(MODULE_ACCESS)); require(am.isOperationAllowed(operation, _msgSender()), "AccessChecker: operation not allowed"); _; + uint256 maxGasLeft = am.maxGasLeft(operation); + if(maxGasLeft > 0) { + require(gasleft() <= maxGasLeft, "Too many gas left"); + } } } \ No newline at end of file diff --git a/contracts/modules/access/AccessModule.sol b/contracts/modules/access/AccessModule.sol index ff25e8c..096c1f3 100644 --- a/contracts/modules/access/AccessModule.sol +++ b/contracts/modules/access/AccessModule.sol @@ -11,6 +11,7 @@ contract AccessModule is Module, IAccessModule, Pausable, WhitelistedRole { bool public whitelistEnabledForAll; bool public whitelistEnabledForIntermediateSenders; + mapping(Operation=>uint256) public maxGasLeft; //Zero value means no limit function initialize(address _pool) public initializer { Module.initialize(_pool); @@ -28,6 +29,10 @@ contract AccessModule is Module, IAccessModule, Pausable, WhitelistedRole { emit WhitelistForIntermediateSendersStatusChange(enabled); } + function setMaxGasLeft(Operation operation, uint256 value) public onlyWhitelistAdmin { + maxGasLeft[operation] = value; + } + function isOperationAllowed(Operation operation, address sender) public view returns(bool) { (operation); //noop to prevent compiler warning if (paused()) return false; From 3ada2193c8b8dea03aa459945aebdc2fa95e9aff Mon Sep 17 00:00:00 2001 From: Pavel Rubin Date: Tue, 17 Nov 2020 16:29:48 +0300 Subject: [PATCH 4/7] Fix compilation errors (wip) --- contracts/interfaces/access/IAccessModule.sol | 2 ++ contracts/modules/access/AccessChecker.sol | 2 +- contracts/modules/access/AccessModule.sol | 8 ++++++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/contracts/interfaces/access/IAccessModule.sol b/contracts/interfaces/access/IAccessModule.sol index ac18cf5..4ee7d70 100644 --- a/contracts/interfaces/access/IAccessModule.sol +++ b/contracts/interfaces/access/IAccessModule.sol @@ -12,4 +12,6 @@ interface IAccessModule { * @param sender Sender of transaction */ function isOperationAllowed(Operation operation, address sender) external view returns(bool); + + function getMaxGasLeft(Operation operation) external view returns(uint256); } \ No newline at end of file diff --git a/contracts/modules/access/AccessChecker.sol b/contracts/modules/access/AccessChecker.sol index 90c5200..f8ad5fc 100644 --- a/contracts/modules/access/AccessChecker.sol +++ b/contracts/modules/access/AccessChecker.sol @@ -9,7 +9,7 @@ contract AccessChecker is Module { IAccessModule am = IAccessModule(getModuleAddress(MODULE_ACCESS)); require(am.isOperationAllowed(operation, _msgSender()), "AccessChecker: operation not allowed"); _; - uint256 maxGasLeft = am.maxGasLeft(operation); + uint256 maxGasLeft = am.getMaxGasLeft(operation); if(maxGasLeft > 0) { require(gasleft() <= maxGasLeft, "Too many gas left"); } diff --git a/contracts/modules/access/AccessModule.sol b/contracts/modules/access/AccessModule.sol index 096c1f3..6f0dcd4 100644 --- a/contracts/modules/access/AccessModule.sol +++ b/contracts/modules/access/AccessModule.sol @@ -11,7 +11,7 @@ contract AccessModule is Module, IAccessModule, Pausable, WhitelistedRole { bool public whitelistEnabledForAll; bool public whitelistEnabledForIntermediateSenders; - mapping(Operation=>uint256) public maxGasLeft; //Zero value means no limit + mapping(uint8=>uint256) public maxGasLeft; //Zero value means no limit function initialize(address _pool) public initializer { Module.initialize(_pool); @@ -30,7 +30,11 @@ contract AccessModule is Module, IAccessModule, Pausable, WhitelistedRole { } function setMaxGasLeft(Operation operation, uint256 value) public onlyWhitelistAdmin { - maxGasLeft[operation] = value; + maxGasLeft[uint8(operation)] = value; + } + + function getMaxGasLeft(Operation operation) public view returns(uint256) { + return maxGasLeft[uint8(operation)]; } function isOperationAllowed(Operation operation, address sender) public view returns(bool) { From ba422b6689e947d74259d3e6579f4c0c10306b7e Mon Sep 17 00:00:00 2001 From: Pavel Rubin Date: Tue, 17 Nov 2020 19:38:28 +0300 Subject: [PATCH 5/7] Fix stack too deep error --- contracts/modules/savings/SavingsModule.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/modules/savings/SavingsModule.sol b/contracts/modules/savings/SavingsModule.sol index 3482525..57eb360 100644 --- a/contracts/modules/savings/SavingsModule.sol +++ b/contracts/modules/savings/SavingsModule.sol @@ -384,13 +384,13 @@ contract SavingsModule is Module, AccessChecker, RewardDistributions, CapperRole uint256 yield; uint256 actualAmount; - uint256 fee; + //uint256 fee; if(nBalanceAfter.add(nAmount) > nBalanceBefore) { yield = nBalanceAfter.add(nAmount).sub(nBalanceBefore); actualAmount = nAmount; }else{ actualAmount = nBalanceBefore.sub(nBalanceAfter); - if (actualAmount > nAmount) fee = actualAmount-nAmount; + //if (actualAmount > nAmount) fee = actualAmount-nAmount; } require(maxNAmount == 0 || actualAmount <= maxNAmount, "SavingsModule: provided maxNAmount is too low"); @@ -405,7 +405,7 @@ contract SavingsModule is Module, AccessChecker, RewardDistributions, CapperRole PoolToken poolToken = PoolToken(protocols[_protocol].poolToken); poolToken.burnFrom(_msgSender(), actualAmount); emit WithdrawToken(_protocol, token, dnAmount); - emit Withdraw(_protocol, _msgSender(), actualAmount, fee); + emit Withdraw(_protocol, _msgSender(), actualAmount, /*fee*/ (actualAmount>nAmount)?actualAmount.sub(nAmount):0 ); if (yield > 0) { From 65b644a74d8b683ebbcc5386533db5776e3a2619 Mon Sep 17 00:00:00 2001 From: Alexander Mazaletskiy Date: Wed, 18 Nov 2020 12:33:11 +0300 Subject: [PATCH 6/7] add crytic-config.json --- crytic-config.json | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 crytic-config.json diff --git a/crytic-config.json b/crytic-config.json new file mode 100644 index 0000000..613ff51 --- /dev/null +++ b/crytic-config.json @@ -0,0 +1,20 @@ +{ + "cwd": "", + "node_version": "12.19.0", + "solc": "0.6.12", + "use_yarn": true, + "custom_unit_test": { + "enabled": true, + "commands": [ + { + "command": ["yarn", "test"] + } + ] + }, + "slither": { + "target": "." + }, + "echidna": { + "contract": "" + } +} From c5f586fb1eadaa22cbe9e8e0699f1f33cea7f6ec Mon Sep 17 00:00:00 2001 From: Alexander Mazaletskiy Date: Wed, 18 Nov 2020 12:34:54 +0300 Subject: [PATCH 7/7] crytic update --- crytic-config.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crytic-config.json b/crytic-config.json index 613ff51..005fc1d 100644 --- a/crytic-config.json +++ b/crytic-config.json @@ -1,7 +1,7 @@ { "cwd": "", "node_version": "12.19.0", - "solc": "0.6.12", + "solc": "0.5.17", "use_yarn": true, "custom_unit_test": { "enabled": true,