Platform: Code4rena
Start Date: 25/08/2022
Pot Size: $75,000 USDC
Total HM: 35
Participants: 147
Period: 7 days
Judge: 0xean
Total Solo HM: 15
Id: 156
League: ETH
Rank: 32/147
Findings: 3
Award: $601.72
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: carlitox477
https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L279 https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Governance.sol#L285
Detailed description of the impact of this finding.
the executeProposal function in Governance contract is vulnerable to reentrancy.
/// @notice Execute the currently active proposal. function executeProposal() external { uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] - noVotesForProposal[activeProposal.proposalId]; if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) { revert NotEnoughVotesToExecute(); } if (block.timestamp < activeProposal.activationTimestamp + EXECUTION_TIMELOCK) { revert ExecutionTimelockStillActive(); } Instruction[] memory instructions = INSTR.getInstructions(activeProposal.proposalId); for (uint256 step; step < instructions.length; ) { kernel.executeAction(instructions[step].action, instructions[step].target); unchecked { ++step; } } emit ProposalExecuted(activeProposal.proposalId); // deactivate the active proposal activeProposal = ActivatedProposal(0, 0); }
note the activeProposal becomes deactive after
kernel.executeAction(instructions[step].action, instructions[step].target) is called.
so the kernel.executeAction can repeated to executeProposal to execute it multiple times.
It is violates the check effect pattern (update state before transaction)
which is vulnerable to reentrancy.
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
After a vote is passed, anyone can execute the proposal by calling
executeProposal
because the proposal is deactivated after
kernel.executeAction(instructions[step].action, instructions[step].target);
kernel.executeAction can call back executeProposal to excute proposal multiple time before the proposal is deactivated.
https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html
check the return value of the executeAction, deactivate the proposal first, use reentrancyGuard modifier to make sure the function is not reentrant.
activeProposal = ActivatedProposal(0, 0); (bool success, ) = kernel.executeAction(instructions[step].action, instructions[step].target); require(success, 'executed faild')
#0 - fullyallocated
2022-09-04T03:13:21Z
Duplicate of #132
🌟 Selected for report: zzzitron
Also found by: 0x040, 0x1f8b, 0x52, 0x85102, 0xDjango, 0xNazgul, 0xNineDec, 0xSky, 0xSmartContract, 0xkatana, 8olidity, Aymen0909, Bahurum, BipinSah, Bnke0x0, CRYP70, CertoraInc, Ch_301, Chandr, Chom, CodingNameKiki, Deivitto, DimSon, Diraco, ElKu, EthLedger, Funen, GalloDaSballo, Guardian, IllIllI, JansenC, Jeiwan, Lambda, LeoS, Margaret, MasterCookie, PPrieditis, PaludoX0, Picodes, PwnPatrol, RaymondFam, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, StevenL, The_GUILD, TomJ, Tomo, Trust, Waze, __141345__, ajtra, ak1, apostle0x01, aviggiano, bin2chen, bobirichman, brgltd, c3phas, cRat1st0s, carlitox477, cccz, ch13fd357r0y3r, cloudjunky, cryptphi, csanuragjain, d3e4, datapunk, delfin454000, devtooligan, dipp, djxploit, durianSausage, eierina, enckrish, erictee, fatherOfBlocks, gogo, grGred, hansfriese, hyh, ignacio, indijanc, itsmeSTYJ, ladboy233, lukris02, martin, medikko, mics, natzuu, ne0n, nxrblsrpr, okkothejawa, oyc_109, p_crypt0, pfapostol, prasantgupta52, rajatbeladiya, rbserver, reassor, ret2basic, robee, rokinot, rvierdiiev, shenwilly, sikorico, sorrynotsorry, tnevler, tonisives, w0Lfrum, yixxas
54.5761 DAI - $54.58
When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.
function setSpreads(uint256 cushionSpread_, uint256 wallSpread_)
function setThresholdFactor(uint256 thresholdFactor_) external onlyRole("operator_policy") {
function setBondContracts(IBondAuctioneer auctioneer_, IBondCallback callback_)
function toggleActive() external onlyRole("operator_admin") {
function resetBeat() external onlyRole("heart_admin") {
function toggleBeat() external onlyRole("heart_admin") {
function toggleBeat() external onlyRole("heart_admin") {
approving the maximum value of uint26 is a known practive to safe gas.
However , this pattern was proven to increase the impact of an attack many times in the pass.
in case the approved contract get hacked.
We recommand approving the exact amount that's needed to be transfered
or add an external function that allows the revovaction of approvals.
ohm.safeApprove(address(MINTR), type(uint256).max);
TRSRY.setApprovalFor(address(this), payoutToken, type(uint256).max);
if (approval != type(uint256).max) { unchecked { withdrawApproval[withdrawer_][token_] = approval - amount_; } }
we recommand that we handle the external function call return value in case of the slient failure of an external function.
We recommand use safeTransferFrom or safeTransfer instead of transfer or transferFrom
VOTES.transferFrom(msg.sender, address(this), userVotes);
VOTES.transferFrom(address(this), msg.sender, userVotes);
🌟 Selected for report: pfapostol
Also found by: 0x040, 0x1f8b, 0x85102, 0xDjango, 0xNazgul, 0xNineDec, 0xSmartContract, 0xkatana, Amithuddar, Aymen0909, Bnke0x0, CertoraInc, Chandr, CodingNameKiki, Deivitto, Dionysus, Diraco, ElKu, Fitraldys, Funen, GalloDaSballo, Guardian, IllIllI, JC, JansenC, Jeiwan, LeoS, Metatron, Noah3o6, RaymondFam, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, Ruhum, Saintcode_, Shishigami, Sm4rty, SooYa, StevenL, Tagir2003, The_GUILD, TomJ, Tomo, Waze, __141345__, ajtra, apostle0x01, aviggiano, bobirichman, brgltd, c3phas, cRat1st0s, carlitox477, cccz, ch0bu, chrisdior4, d3e4, delfin454000, djxploit, durianSausage, erictee, exolorkistis, fatherOfBlocks, gogo, grGred, hyh, ignacio, jag, karanctf, kris, ladboy233, lukris02, m_Rassska, martin, medikko, natzuu, ne0n, newfork01, oyc_109, peiw, rbserver, ret2basic, robee, rokinot, rvierdiiev, sikorico, simon135, tnevler, zishansami
32.6845 DAI - $32.68
When using elements that are smaller than 32 bytes, your contract�s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
function VERSION() external pure virtual returns (uint8 major, uint8 minor) {}
function VERSION() external pure override returns (uint8 major, uint8 minor) {
function VERSION() external pure override returns (uint8 major, uint8 minor) {
uint48 lastActive; // Unix timestamp when the side was last active (in seconds)
lastActive: uint48(block.timestamp),
lastActive: uint48(block.timestamp),
function VERSION() external pure override returns (uint8 major, uint8 minor) {
_range.high.lastActive = uint48(block.timestamp);
_range.low.lastActive = uint48(block.timestamp);
lastActive: uint48(block.timestamp),
lastActive: uint48(block.timestamp),
event MovingAverageDurationChanged(uint48 movingAverageDuration_);
event ObservationFrequencyChanged(uint48 observationFrequency_);
uint32 public nextObsIndex;
uint32 public numObservations;
uint48 public observationFrequency;
uint48 public movingAverageDuration;
uint48 public lastObservationTime;
uint8 public constant decimals = 18;
uint48 observationFrequency_,
uint48 movingAverageDuration_
uint8 ohmEthDecimals = _ohmEthPriceFeed.decimals();
uint8 reserveEthDecimals = _reserveEthPriceFeed.decimals();
numObservations = uint32(movingAverageDuration_ / observationFrequency_);
function VERSION() external pure override returns (uint8 major, uint8 minor) {
uint32 numObs = numObservations;
lastObservationTime = uint48(block.timestamp);
uint32 lastIndex = nextObsIndex == 0 ? numObservations - 1 : nextObsIndex - 1;
if (startObservations_.length != numObs || lastObservationTime_ > uint48(block.timestamp))
function changeMovingAverageDuration(uint48 movingAverageDuration_) external permissioned {
numObservations = uint32(newObservations);
function changeObservationFrequency(uint48 observationFrequency_) external permissioned {
numObservations = uint32(newObservations);
function VERSION() external pure override returns (uint8 major, uint8 minor) {
function VERSION() public pure override returns (uint8 major, uint8 minor) {
event CushionFactorChanged(uint32 cushionFactor_);
event CushionParamsChanged(uint32 duration_, uint32 debtBuffer_, uint32 depositInterval_);
event ReserveFactorChanged(uint32 reserveFactor_);
event RegenParamsChanged(uint32 wait_, uint32 threshold_, uint32 observe_);
uint8 public immutable ohmDecimals;
uint8 public immutable reserveDecimals;
uint32 public constant FACTOR_SCALE = 1e4;
uint32[8] memory configParams // [cushionFactor, cushionDuration, cushionDebtBuffer, cushionDepositInterval, reserveFactor, regenWait, regenThreshold, regenObserve]
if (configParams[2] < uint32(10_000)) revert Operator_InvalidParams();
if (configParams[3] < uint32(1 hours) || configParams[3] > configParams[1])
configParams[7] == uint32(0)
count: uint32(0),
lastRegen: uint48(block.timestamp),
nextObservation: uint32(0),
uint48(block.timestamp) >= RANGE.lastActive(true) + uint48(config_.regenWait) &&
uint48(block.timestamp) >= RANGE.lastActive(false) + uint48(config_.regenWait) &&
uint8(
vesting: uint48(0), // Instant swaps
conclusion: uint48(block.timestamp + config_.cushionDuration),
uint8 oracleDecimals = PRICE.decimals();
uint8(
vesting: uint48(0), // Instant swaps
conclusion: uint48(block.timestamp + config_.cushionDuration),
function setCushionFactor(uint32 cushionFactor_) external onlyRole("operator_policy") {
uint32 duration_,
uint32 debtBuffer_,
uint32 depositInterval_
if (debtBuffer_ < uint32(10_000)) revert Operator_InvalidParams();
if (depositInterval_ < uint32(1 hours) || depositInterval_ > duration_)
function setReserveFactor(uint32 reserveFactor_) external onlyRole("operator_policy") {
uint32 wait_,
uint32 threshold_,
uint32 observe_
uint32 observe = _config.regenObserve;
_status.high.count = uint32(0);
_status.high.nextObservation = uint32(0);
_status.high.lastRegen = uint48(block.timestamp);
_status.low.count = uint32(0);
_status.low.nextObservation = uint32(0);
_status.low.lastRegen = uint48(block.timestamp);
function changeMovingAverageDuration(uint48 movingAverageDuration_)
function changeObservationFrequency(uint48 observationFrequency_)
// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from �false� to �true�, after having been �true� in the past
bool public isActive;
function setActiveStatus(bool activate_) external onlyKernel {
mapping(Keycode => mapping(Policy => mapping(bytes4 => bool))) public modulePermissions;
mapping(address => mapping(Role => bool)) public hasRole;
mapping(Role => bool) public isRole;
bool granted_
bool grant_
event WallUp(bool high_, uint256 timestamp_, uint256 capacity_);
event WallDown(bool high_, uint256 timestamp_, uint256 capacity_);
event CushionUp(bool high_, uint256 timestamp_, uint256 capacity_);
event CushionDown(bool high_, uint256 timestamp_);
bool active; // Whether or not the side is active (i.e. the Operator is performing market operations on this side, true = active, false = inactive)
function updateCapacity(bool high_, uint256 capacity_) external permissioned {
function regenerate(bool high_, uint256 capacity_) external permissioned {
bool high_,
function capacity(bool high_) external view returns (uint256) {
function active(bool high_) external view returns (bool) {
function price(bool wall_, bool high_) external view returns (uint256) {
function spread(bool wall_) external view returns (uint256) {
function market(bool high_) external view returns (uint256) {
function lastActive(bool high_) external view returns (uint256) {
bool public initialized;
function transfer(address to_, uint256 amount_) public pure override returns (bool) {
) public override permissioned returns (bool) {
bool public initialized;
bool public active;
observations: new bool[](configParams[7])
function _activate(bool high_) internal {
function _deactivate(bool high_) internal {
_status.high.observations = new bool[](observe_);
_status.low.observations = new bool[](observe_);
function regenerate(bool high_) external onlyRole("operator_admin") {
function _updateCapacity(bool high_, uint256 reduceBy_) internal {
function _regenerate(bool high_) internal {
_status.high.observations = new bool[](_config.regenObserve);
_status.low.observations = new bool[](_config.regenObserve);
function _checkCushion(bool high_) internal {
bool sideActive = RANGE.active(high_);
function fullCapacity(bool high_) public view override returns (uint256) {
mapping(address => mapping(uint256 => bool)) public approvedMarkets;
bool public active;
event WalletVoted(uint256 proposalId, address voter, bool for_, uint256 userVotes);
mapping(uint256 => bool) public proposalHasBeenActivated;
mapping(uint256 => mapping(address => bool)) public tokenClaimsForProposal;
function vote(bool for_) external {
for (uint256 i = 0; i < reqLength; ) {
for (uint256 i = 0; i < 5; ) {
for (uint256 i = 0; i < 32; ) {
function grantRole(Role role_, address addr_) public onlyAdmin {
function revokeRole(Role role_, address addr_) public onlyAdmin {
function withdrawReserves(
function mintOhm(address to_, uint256 amount_) public permissioned {
function burnOhm(address from_, uint256 amount_) public permissioned {
function updateMarket(
function getInstructions(uint256 instructionsId_) public view returns (Instruction[] memory) {
function getMetadata(uint256 proposalId_) public view returns (ProposalMetadata memory) {
function getActiveProposal() public view returns (ActivatedProposal memory) {
The overheads outlined below are PER LOOP, excluding the first loop
storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
for (uint256 step; step < instructions.length; ) {