Reserve contest - yongskiws's results

[L-01] Potential DOS in Contract Inheriting UUPSUpgradeable.sol

As a result, a malicious attacker could monitor the Ethereum blockchain for bytecode that matches the contract and frontrun the initialize() transaction to gain ownership of the contract. This can be repeated as a Denial Of Service (DOS) type of attack, effectively preventing contract deployment, leading to unrecoverable gas expenses.

Add a control that makes initialize() only call the Deployer Contract;

if (msg.sender != DEPLOYER_ADDRESS) {
            revert NotDeployer();
        }

contracts\p1\Main.sol
26:     function init(
27:         Components memory components,
28:         IERC20 rsr_,
29:         uint48 shortFreeze_,
30:         uint48 longFreeze_
31:     ) public virtual initializer {
32:         require(address(rsr_) != address(0), "invalid RSR address");
33:         __Auth_init(shortFreeze_, longFreeze_);
34:         __ComponentRegistry_init(components);
35:         __UUPSUpgradeable_init();
36: 
37:         rsr = rsr_;
38:         emit MainInitialized();
39:     }

[L-02] initialize() function can be called by anybody

initialize() function can be called anybody when the contract is not initialized.

More importantly, if someone else runs this function. Also, there is no 0 address check in the address arguments of the initialize() function, which must be defined.

Add a control that makes initialize() only call the Deployer Contract;

if (msg.sender != DEPLOYER_ADDRESS) {
            revert NotDeployer();
}

contracts\p1\BackingManager.sol
44:  function init(
45:         IMain main_,
46:         uint48 tradingDelay_,
47:         uint192 backingBuffer_,
48:         uint192 maxTradeSlippage_,
49:         uint192 minTradeVolume_
50:     ) external initializer {
51:         __Component_init(main_);
52:         __Trading_init(main_, maxTradeSlippage_, minTradeVolume_);
53: 
54:         assetRegistry = main_.assetRegistry();
55:         basketHandler = main_.basketHandler();
56:         distributor = main_.distributor();
57:         rsr = main_.rsr();
58:         rsrTrader = main_.rsrTrader();
59:         rTokenTrader = main_.rTokenTrader();
60:         rToken = main_.rToken();
61:         stRSR = main_.stRSR();
62: 
63:         setTradingDelay(tradingDelay_);
64:         setBackingBuffer(backingBuffer_);
65:     }

contracts\p1\Broker.sol
51:     function init(
52:         IMain main_,
53:         IGnosis gnosis_,
54:         ITrade tradeImplementation_,
55:         uint48 auctionLength_
56:     ) external initializer {
57:         require(address(gnosis_) != address(0), "invalid Gnosis address");
58:         require(
59:             address(tradeImplementation_) != address(0),
60:             "invalid Trade Implementation address"
61:         );
62:         __Component_init(main_);
63: 
64:         backingManager = main_.backingManager();
65:         rsrTrader = main_.rsrTrader();
66:         rTokenTrader = main_.rTokenTrader();
67: 
68:         gnosis = gnosis_;
69:         tradeImplementation = tradeImplementation_;
70:         setAuctionLength(auctionLength_);
71:     }

contracts\p1\Furnace.sol
33:     function init(
34:         IMain main_,
35:         uint48 period_,
36:         uint192 ratio_
37:     ) external initializer {
38:         __Component_init(main_);
39:         rToken = main_.rToken();
40:         setPeriod(period_);
41:         setRatio(ratio_);
42:         lastPayout = uint48(block.timestamp);
43:         lastPayoutBal = rToken.balanceOf(address(this));
44:     }

contracts\p1\RevenueTrader.sol
23:     function init(
24:         IMain main_,
25:         IERC20 tokenToBuy_,
26:         uint192 maxTradeSlippage_,
27:         uint192 minTradeVolume_
28:     ) external initializer {
29:         require(address(tokenToBuy_) != address(0), "invalid token address");
30:         __Component_init(main_);
31:         __Trading_init(main_, maxTradeSlippage_, minTradeVolume_);
32:         assetRegistry = main_.assetRegistry();
33:         distributor = main_.distributor();
34:         tokenToBuy = tokenToBuy_;
35:     }

contracts\p1\RToken.sol
147:     function init(
148:         IMain main_,
149:         string calldata name_,
150:         string calldata symbol_,
151:         string calldata mandate_,
152:         uint192 issuanceRate_,
153:         uint192 scalingRedemptionRate_,
154:         uint256 redemptionRateFloor_
155:     ) external initializer {
156:         require(bytes(name_).length > 0, "name empty");
157:         require(bytes(symbol_).length > 0, "symbol empty");
158:         require(bytes(mandate_).length > 0, "mandate empty");
159:         __Component_init(main_);
160:         __ERC20_init(name_, symbol_);
161:         __ERC20Permit_init(name_);
162: 
163:         assetRegistry = main_.assetRegistry();
164:         basketHandler = main_.basketHandler();
165:         backingManager = main_.backingManager();
166:         furnace = main_.furnace();
167: 
168:         mandate = mandate_;
169:         setIssuanceRate(issuanceRate_);
170:         setScalingRedemptionRate(scalingRedemptionRate_);
171:         setRedemptionRateFloor(redemptionRateFloor_);
172:     }

contracts\p1\StRSR.sol
160:     function init(
161:         IMain main_,
162:         string calldata name_,
163:         string calldata symbol_,
164:         uint48 unstakingDelay_,
165:         uint48 rewardPeriod_,
166:         uint192 rewardRatio_
167:     ) external initializer {
168:         require(bytes(name_).length > 0, "name empty");
169:         require(bytes(symbol_).length > 0, "symbol empty");
170:         __Component_init(main_);
171:         __EIP712_init(name_, "1");
172:         name = name_;
173:         symbol = symbol_;
174: 
175:         assetRegistry = main_.assetRegistry();
176:         backingManager = main_.backingManager();
177:         basketHandler = main_.basketHandler();
178:         rsr = IERC20(address(main_.rsr()));
179: 
180:         payoutLastPaid = uint48(block.timestamp);
181:         rsrRewardsAtLastPayout = main_.rsr().balanceOf(address(this));
182:         setUnstakingDelay(unstakingDelay_);
183:         setRewardPeriod(rewardPeriod_);
184:         setRewardRatio(rewardRatio_);
185: 
186:         beginEra();
187:         beginDraftEra();
188:     }

[L-3] require() should be used instead of assert()

contracts\plugins\trading\GnosisTrade.sol
63:         assert(status == TradeStatus.PENDING);
contracts\plugins\trading\GnosisTrade.sol
98:         assert(origin_ != address(0));
contracts\plugins\trading\GnosisTrade.sol
182:          assert(isAuctionCleared());
contracts\plugins\assets\RTokenAsset.sol
74:  assert(low <= high);
contracts\plugins\assets\FiatCollateral.sol
137:        assert(low == 0);
contracts\plugins\assets\Asset.sol
147:         assert(lotLow <= lotHigh);
contracts\plugins\assets\Asset.sol
112:   assert(low <= high);
contracts\plugins\assets\Asset.sol
98:     assert(low == 0);
contracts\plugins\aave\StaticATokenLM.sol
345:         assert(amt == amountToWithdraw);
contracts\p1\mixins\Trading.sol
114:         assert(address(trades[sell]) == address(0));
contracts\p1\mixins\TradeLib.sol
170:     assert(keccak256(reason) == UIntOutofBoundsHash);
contracts\p1\mixins\TradeLib.sol
168:     assert(errorCode == 0x11 || errorCode == 0x12);
contracts\p1\mixins\TradeLib.sol
108:         assert(
109:             trade.sellPrice > 0 &&
110:                 trade.sellPrice < FIX_MAX &&
111:                 trade.buyPrice > 0 &&
112:                 trade.buyPrice < FIX_MAX
113:         );
contracts\p1\mixins\TradeLib.sol
44:         assert(trade.buyPrice > 0 && trade.buyPrice < FIX_MAX && trade.sellPrice < FIX_MAX);
45: 
contracts\p1\mixins\RewardableLib.sol
102:             assert(erc20.balanceOf(address(this)) >= liabilities[erc20]);
contracts\p1\mixins\RewardableLib.sol
78:                 assert(erc20s[i].balanceOf(address(this)) >= liabilities[erc20s[i]]);
contracts\p1\mixins\RecollateralizationLib.sol
110:         assert(doTrade);
contracts\p1\StRSR.sol
696:         assert(totalStakes + amount < type(uint224).max);
contracts\p1\BasketHandler.sol
556:             assert(targetIndex < targetsLength);
contracts\p1\BackingManager.sol
249:         assert(tradesOpen == 0 && !basketHandler.fullyCollateralized());

[L-4] Functions return bool but cannot return false

contracts\plugins\mocks\vendor\EasyAuction.sol
313:        return true;
contracts\plugins\mocks\vendor\EasyAuction.sol
298:         return true;
contracts\plugins\mocks\WETH.sol
56:         return true;
contracts\plugins\mocks\RTokenCollateral.sol
81:         return true;
contracts\plugins\assets\FiatCollateral.sol
221:         return true;
contracts\p1\StRSR.sol
665:        return true;
contracts\p1\StRSR.sol
654:         return true;
contracts\p1\StRSR.sol
648:         return true;
contracts\p1\StRSR.sol
634:         return true;
contracts\p1\StRSR.sol
625:         return true;
contracts\p1\AssetRegistry.sol
176:     return true;

[NC-01] Not using the latest version of OpenZeppelin from dependencies and Solidity 0.8.*

@openzeppelin/contracts": "^4.7.3
@openzeppelin/contracts-upgradeable": "^4.7.3

[NC-02] getPastVotes and getPastTotalSupply must have some validation to prevent errors

users who try to access the voting power and total supply data from the past voting power by using an invalid blockNumber, then the getPastVotes function will not be able to provide the appropriate data and this can cause security problems.The same goes for the getPastTotalSupply function . EG.

    require(_checkpoints[pastEra][account].length > 0, "ERC20Votes: account has no voting power during the specified era");
    require(blockNumber > 0, "ERC20Votes: invalid block number");
    require(pastEra < _eras.length, "ERC20Votes: invalid era");

contracts\p1\StRSRVotes.sol
76:     function getPastVotes(address account, uint256 blockNumber) public view returns (uint256) {
77:         require(blockNumber < block.number, "ERC20Votes: block not yet mined");
78:         uint256 pastEra = _checkpointsLookup(_eras, blockNumber);
79:         return _checkpointsLookup(_checkpoints[pastEra][account], blockNumber);
80:     }
81: 
82:     function getPastTotalSupply(uint256 blockNumber) public view returns (uint256) {
83:         require(blockNumber < block.number, "ERC20Votes: block not yet mined");
84:         uint256 pastEra = _checkpointsLookup(_eras, blockNumber);
85:         return _checkpointsLookup(_totalSupplyCheckpoints[pastEra], blockNumber);
86:     }