5.2 Highs
Last updated
Last updated
This finding was a Consensys Diligence audit of 1inch protocol where it was a high severity finding related to privileged roles and timing, where there could be unpredictable behavior for users due to admin Front-running or in general.
For context, administrators of contracts could update or upgrade things in the system without warning, which could violate security goals of the system. Specifically, privileged roles could use front-running to make malicious changes just ahead of incoming transactions or purely accidental negative effects that could occur due to unfortunate timing of such changes.
The recommendation was to give users advanced notice of changes with the time lock, for example by making all system parameter and upgrades to require two steps, with a mandatory time window between them.
The first step would broadcast to users that a particular change was coming.
The second step would be committing that change after a suitable waiting period.
This would allow users that do not want to accept such change to exit, and others who are okay with those changes to continue engaging with the protocol.
This is related to OpenZeppelin's TimelockController library we discussed in 182 or Solidity 201 module and two-step change of privileged roles in 162 time delay change of critical parameters and 163 along with broader aspects of timing in 177 and trust issues in 181 that we discussed in security pitfalls and best practices 201 model.
This finding was a ConsenSys Diligence audit of the DeFi Saver protocol. This was a major severity finding in the input validation category, where tokens with more than 18 decimal points could have caused issues. The code assumed that the maximum number of decimals for each token was 18. However, although this is uncommon, it is possible to have tokens with more than 18 decimals (for example Yam V2 has 24 decimals) and interacting with such tokens could have resulted in broken code flow and unpredictable outcomes.\
The specific recommendation was to make sure that the code won't fail in case the token's decimals were more than 18 and was fixed by using SafeMath's sub function to revert to tokens that have greater than 18 decimals. We have discussed this aspect of ERC20 decimals in , more specifically the . This issue is also relevant in .
Another major severity finding from ConsenSys Diligence audit of the DeFi Saver protocol was in the error handling category, where error codes of a few compound protocol functions called by these contracts were not checked. Some of compound's protocols functions return an error code instead of reverting in case of failure, but DeFi Saver contracts never checked for error codes returned from such functions, causing them to not react to exceptional conditions in such function calls by reverting or other means necessary.\
The specific recommendation was for the caller contract to revert in case the error code returned was not zero, indicating a failure. This was fixed accordingly. We've discussed this aspect of checking for function return values at number 142 and 175 of security pitfalls and best practices 201 module.
This finding was a ConsenSys Diligence audit of the DAOFI protocol where it was a major severity finding in the error handling category. The error was that swapExactTokensForETH checked the wrong return value instead of checking that the amount of tokens received from a swap was greater than the minimum amount expected from the swap. It calculated the difference between the initial receiver's balance and the balance of the router.
The recommendation was to check the intended values. We have discussed this aspect of correctly checking function return values in number 142 and 175 of security pitfalls and best practices 201 module.
This finding was a ConsenSys Diligence audit of Fei protocol where it was a major severity finding related to the data validation category. Fei performed some mint/burn operations via UniswapIncentive.incentivize function, which calculated buy/sell incentives using overflow-prone map, then minted/burned from the target based on the results. Any overflows would have had unintended consequences on such minting or burning. The specific overflow prone map was because of unsafe casting from a user-supplied uint256 argument in the externally visible function to int256, which is a downcast and may have overflowed without appropriate checks.
The recommendation was to ensure that casts do not overflow, and was addressed by the use of OpenZeppelin's SafeCast. This finding is related to OpenZeppelin's SafeCast wrappers we discussed in number 177 of Solidity 201 module, dangers of integer overflow underflow we discussed in number 19 of security pitfalls and best practices 101 module, the broader aspect of the importance of input validation specifically on function parameters in 138 and 146 of security pitfalls and best practices 201 module.
This finding was a ConsenSys Diligence audit of bitbank protocol where it was a major severity finding related to error handling category. In this case, ERC20 tokens that did not return a value would fail to transfer. Remember that although the ERC20 standard suggests that a transfer should return true on success, some tokens may be non-compliant and in such a case the transfer call here would revert even if it were successful because of Solidity checking that the return data size matches the ERC20 interface.
The recommendation was to consider using OpenZeppelin's safeERC20 wrappers. We have specifically discussed this in number 149 Solidity 201 module and number 24 are security pitfalls and best practices 101 module.
This finding was a ConsenSys Diligence audit of MetaSwap protocol where it was a major severity finding related to reentrancy. This reentrancy vulnerability was a MetaSwap.swap function where, if an attacker was able to reenter swap, they could execute their own trade using the same tokens and get all the tokens for themselves.
The recommendation was to add ReentrancyGuard such as the one from OpenZeppelin, where the NonReentrant modifiers are used on functions such as MetaSwap.swap that may be vulnerable to reentrances. We have discussed these aspects in number 157 of Solidity 201 and number 13 of security pitfalls and best practices 101 modules.
This finding was a ConsenSys Diligence audit of mstable protocol where it was a major severity finding related to the timing category. In this case, it was the views of a sliding window where users could collect interest by only staking mTokens momentarily. For more context, when users deposited m-assets, in return for lending yield and swap fees users received credit tokens at an exchange rate which was updated at every deposit. However, it enforced a minimum time frame of 30 minutes in which the interest rate would not be updated. A user who deposited shortly before the end of the time frame would receive credits at the stale interest rate and could immediately trigger an update of the rate and withdraw at the updated and more favorable rate after the 30 minute window. As a result, it would be possible for users to benefit from interest payouts by only staking m-assets momentarily.
The recommendation was to remove the 30 minutes window such that every deposit also updated the exchange rate between credits and tokens. This finding is therefore related to the timing issues discussed in number 143 and 177 of security pitfalls and best practices 201 module where system design has to anticipate and prevent abuse of timing aspects and assumptions of the protocol.
This finding was a ConsenSys Diligence audit of Shell protocol where it was a major severity finding related to the input validation category, where certain functions lack input validation such as: uint should be larger than 0 when 0 is considered invalid, uint should be within constraints or thresholds, int should be positive in some cases, length of arrays should match if more than one related arrays are sent as arguments, and addresses should not be zero-addresses.\
The recommendation was to add input validation and incorporate tests that check if all parameters had indeed been validated. We've discussed the importance of input validation on function parameters and arguments in number 138 and 146 of security pitfalls and best practices 201 module.
Another major severity finding from ConsenSys Diligence audit of Shell protocol was related to access control. There were several functions that gave extreme powers to the protocol administrator, of which the most dangerous was the one granting capability where the administrator could intentionally or accidentally deploy malicious or faulty code that could drain the whole pool or lock up the users' and LP's tokens. Also the function safeApprove allowed the administrator to move any of the users' tokens in the contract to any address which could be used as a bad door to completely drain the contract.\
The recommendation was to remove the safeApprove function and improve users trust by making the code static and unchangeable after deployment. We discussed the importance of least privileged principle in number 192 and principle of separation of privilege in number 193 of security pitfalls and best practices 201 module, this also related to access control and trust issues we discussed in number 148, 149, 160 and 172 or security pitfalls and best practices 201 module.
This finding was a ConsenSys Diligence audit of the LAO protocol where it was a major severity finding related to denial of service. In this issue, token overflows might result in system halt or loss of funds because some functionality such as processProposal and cancelProposal would break due to SafeMath reverts. The overflows could happen because the supply of the token was artificially inflated.\
The recommendation was to allow overflows for broken or malicious tokens to prevent system halt or loss of funds, but recognizing that in case such overflows occur, the balance of tokens would be incorrect for all token holders in the system.\
This is again related to the denial of service aspect we discussed in number 176 of security pitfalls and best practices 201e module, and also the dangers of integer overflow underflow we discussed in number 19 our security pitfalls and best practices 101 module.
Another major severity finding from ConsenSys Diligence audit of the LAO protocol was again related to denial of service. The issue was that while iterating over all whitelisted tokens, if the number of tokens was too large, a transaction could run Out-of-Gas and all funds would be blocked forever.\
The recommendation was to limit the number of whitelisted tokens or to add a function to remove tokens from the whitelist.\
This is related to the aspect of loop operations leading to denial of service from Out-of-Gas exception in number 42 and denial of service from block Gas Limit because of looping over a raise of unknown size we discussed in number 44 of security pitfalls and best practices 101 module, the broader aspects of denial of service and number 176 and Gas issues we discussed in number 182 of security pitfalls and best practices 201 module.
Another major severity finding from ConsenSys Diligence audit of the LAO protocol was again related to denial of service. The issue was that the summoner could steal funds using bailout(). The bailout() function allowed anyone to transfer the kicked users' funds to the summoner if the user did not call safeRangequit().\
The intention was for the summoner to then transfer those funds to the kicked member afterwards. But the issue was that it required a lot of trust on the summoner for doing so, and they could deny such a transfer.\
The recommendation again was to use a pull mechanism which would make the bailout function unnecessary. This denial of service aspect is something we discussed in number 176 of security pitfalls and best practices to one module.
Another major severity finding from ConsenSys Diligence audit of the LAO protocol was again related to denial of service. If the proposal submission and sponsorship were done in two different transactions, it was possible to front run the sponsor proposal function by any member, so that they could block the proposal thereafter.\
The recommendation of pull pattern for token transfers would solve the issue by making Front-running ineffective. This is related to the transaction order dependence aspect discussed in number 21 of security pitfalls and best practices 101 module and denial of service aspect that we discussed in number 176 of security pitfalls and best practices 201.
Yet another major severity finding from ConsenSys Diligence audit of the LAO protocol was again related to denial of service. Any member could front run another member's delegate key assignment where if one tried to submit an address as the delegate key, someone else could try to assign that delegate address to themselves making it possible to block some address from being a delegate forever.\
The recommendation was to make it possible to approve delegate key assignment or cancel the current delegation commit. Reveal methods could also be used to mitigate this attack.\
This issue is related to the transaction order dependence aspect discussed in number 21 or security pitfalls and best practices 101 module and denial of service aspect that we discussed in number 176 of security pitfalls and best practices 201.\
An interesting point to note here is that the project decided to not fix this finding as reported in the audit report presumably because they did not see this as a major severity issue in the risk model.\
As discussed in the module on audit techniques and tools 101, the findings and recommendations reported by an audit team may not necessarily be fixed by the project team for different reasons, but usually because they're outside the threat model or within the trust model of the project.
This finding was a Trail of Bits audit of the Origin Dollar where it was a high severity finding related to denial of service. The specific issue was that queued transactions could never be cancelled.\
The governor contract contained special functions to set it as the admin of the timelock and only the admin could call timelock.cancelTransaction(), but there were no functions in governor that called timelock.cancelTransaction() which made it impossible to ever be called.\
The recommendation was in the short term to add a function to the governor that calls timelock.cancelTransaction() and in the long term considering letting governor inherit from timelock, which would allow a lot of code to be removed and significantly lower the complexity of the two contracts.\
This is related to the denial of service aspect that we discussed in number 176 of security pitfalls and best practices 201 module.
Another high severity finding from Trail of Bits audit of the Origin Dollar was related to access control. Missing access control checks in the timelock.executeTransaction() function allowed proposal transactions to be executed separately, circumventing the governor execute() function.\
The recommendation was to allow only the admin to call timelock.executeTransaction. This is related to access control aspects discussed in number 4 of security pitfalls and best practices 101 module and also in number 141, 148, 149, 150 and 172 or security pitfalls and best practices 201 module.
Another high severity finding from Trail of Bits audit of the Origin Dollar was again related to access control. The governor contract contained special functions to let the guardian queue a transaction to change the timelock.\
However a regular proposal was also allowed to contain a setPendingAdmin transaction to change the timelock.admin, which posed an unnecessary risk in that an attacker could create a proposal to change the timelock.admin themselves.\
The recommendation was to add a check that prevented setPendingAdmin to be included in a regular proposal. This is related to access control aspects discussed in number 148, 149 and 172 and also the principle of complete mediation discussed in number 196 of security pitfalls and best practices 201 modules.
Another high severity finding from Trail of Bits audit of the Origin Dollar was related to reentrancy. Missing checks and no reentrancy prevention allowed untrusted contracts to be called from the mintMultiple() function which could be used by an attacker to drain the contract.\
The recommendation was in the short term to add checks to cause mintMultiple() to revert if the amount was zero or the asset was not supported, and also to add a reentrancy guard to the mint(), mintMultiple(), redeem() and redeemAll() functions.\
In the long term, the recommendation was to make use of Slither and incorporate static analysis checks into the CI/CD pipeline, which would have flagged the reentrancy. Other recommendations were to add reentrancy guards to all non-view functions callable by anyone, ensure to always revert a transaction if an input is incorrect and to disallow calling untrusted contracts.\
We have discussed these aspects in number 157 of Solidity 201 and number 13 of security pitfalls and best practices 101 modules.
Another high severity finding from Trail of Bits audit of the Origin Dollar was related to error handling. The issue was that several function calls did not check the return value without which the code is error-prone and may lead to unexpected results.\
The recommendation was to check the return value of all function calls that return a value. We have discussed this in number 74 of security pitfalls and best practices 101 module and number 142 of security pitfalls and best practices 201.
Another high severity finding from Trail of Bits audit of the Origin Dollar was related to denial of service. Several function calls were made in unbounded loops, which is Error-prone because it can trap the contracts due to Gas Limitations or failed transactions.\
The recommendation was to review all the loops to allow iteration over part of the loop, or remove elements depending on Gas consumption to prevent denial of service.\
This is related to the aspect of making external calls within loops leading to denial of service from Out-of-Gas exceptions discussed in number 43 of security pitfalls and best practices 101 module and broader aspects of denial of service in number 176 and Gas issues we discussed in number 182 of security pitfalls and best practices 201.
Another high severity finding from Trail of Bits audit of the Origin Dollar was related to data validation. Under certain circumstances the OUSD contract allowed users to transfer more tokens than they had in their balance, which is caused by a rounding issue.\
The recommendation was in the short term to make sure the balance is correctly checked before performing all the arithmetic operations and in the long term to use the Trail of Bits tool Echidna to write properties on arithmetic invariants that ensure ERC20 transfers are transferring the expected amount.\
This is related to aspects of token handling in number 159 data validation issues number 169 and numerical issues in number 170 we discussed in security pitfalls and best practices 201 module.
Yet nother high severity finding from Trail of Bits audit of the Origin Dollar was again related to data validation. OUSD total supply could be arbitrary and even smaller than user balances because the OUSD token contract allowed users to opt out of rebasing effects, at which point their exchange rate would be fixed and further rebases would not have any impact on token balances until the user opts back in.\
The recommendation was to specify all common invariant violations for users and other stakeholders in the short term and redesign the system in the long run to preserve as many common invariants as possible.\
This is related to aspects of token handling in number 159, data validation issues in number 169 and numerical issues in 170 We discussed in security pitfalls and best practices 201 module.
This finding was a Trail of Bits audit of the Yield protocol where it was a high severity finding related to access control in which a lack of chainID validation allowed signatures to be reused across forks.
YDAI implemented the draft ERC2612 standard which allows a third party to transmit a signature from a token holder that modified the ERC20 allowance for a particular user. These signatures used in calls to permit an ERC20 permit did not account for chain splits and as a result, if the chain forked after deployment, the signed message would be considered valid on both forks.
The recommendation was to include the chainID opcode in the permit schema to make replay attacks impossible in the event of a post-deployment hard fork. This is related to aspects of cryptography issues we discussed in number 174 of security pitfalls and best practices 201 module.
This finding was a Trail of Bits audit of Hermez where it was a high severity finding related to data validation in which lack of a contract existence check effectively allowed token theft.\
The recommendation was to check for contract existence and _safeTransferFrom() function before interacting with the contract. Remember that the Solidity documentation talks about low-level call delegateCall() and callcode() returning true success even if the called account is non-existent.\
This is related to number 87 of Solidity 101 module, number 38 of security pitfalls and best practices 101 module, the broader aspects of external interaction issues we discussed in number 174 of security pitfalls and best practices 201.
Another high severity finding from Trail of Bits audit of the Hermez was related to access control. The system used the same account to change both frequently updated parameters and less frequent ones, which is an Error-prone and risky design, because it increases the severity of any privileged account compromises.\
The recommendation was to use a separate account to handle updating the less frequently changed parameters and in the long term to design and document the access control architecture carefully. This is related to access control aspects discussed in number 148, 149 and 172 and also the principle of least common mechanism we discussed in number 194 of the security pitfalls and best practices 201 module.
Another high severity finding from Trail of Bits audit of the Hermez was related to data validation. The issue was the one-step procedure for critical operations that is Error-prone and can lead to irrecoverable mistakes.
For example, the setter for the white hat group address sets the address to the provided argument, if the address is incorrect, then the new address would take on the functionality of that role, immediately leaving it open to misuse by anyone who controlled that new address or, if the new address was one without an available private key, it would lock access to that role forever. The recommendation was to use a two-step procedure for all such Critical Address updates to prevent irrecoverable mistakes. This is related to the two-step change of privilege rules we discussed in number 50 of security pitfalls and best practices 101 module and number 162 of the security pitfalls and best practices to one module.
Another high severity finding from Trail of Bits audit of the Hermez was related to configuration, in which Hermez auction protocol and withdrawal delayer had initialization functions that could be frontrun due to the use of the delegateCall Proxy pattern.\
These contracts could not be initialized with the constructor and had initializer functions, all of whom could be frontrun by an attacker, allowing them to initialize the contracts with malicious values.\
The recommendation was to either use a factory pattern that would prevent front-running of the initialization or ensure deployment scripts are robust to prevent front-running attacks by atomically deploying and initializing. This was discussed in number 192 of Solidity 201, number 21 and number 95 of security pitfalls and best practices 101 module and number 143 and 166 of the security pitfalls and best practices 201 module.
This finding was a Trail of Bits audit of Uniswap V3 where it was a high severity finding related to data validation in which an incorrect comparison in the swap function allowed the swap to succeed even if no tokens were paired.\
This issue could be used to drain any pool of all of its tokens at no cost. The check inside one of the requires in that function was incorrect because it used >= instead <=.\
The recommendation was to simply replace the >= with <= in that require statement. This is related to the aspect of conditionals we discussed in number 147, the broader aspect of data validation issues in number 169 of security pitfalls and best practices 201 body.
Another high severity finding from Trail of Bits audit of the Uniswap V3 was related to data validation, in which failed transfers may be overlooked due to lack of contract existence check.\
TransferHelper.safeTransfer performed a transfer with a low level call without confirming the contract's existence. As a result, if the tokens had not yet been deployed or had been destroyed, TransferHelper.safeTransfer would still have returned success even though no transfer had actually executed.\
The recommendation was to check for contract existence before interacting with the contract. Remember that the solicited documentation warns about low level call delegateCall and call code returning success even if the called account is non-existent.\
This is related to number 87 or Solidity 101 module, number 38 of security pitfalls and best practices 101 module, the broader aspects of external interaction issues we discussed in number 174 of security pitfalls and best practices 201 module.
This finding was a Trail of Bits audit of DFX protocol where it was a high severity finding related to undefined behavior, in which the left hand side of an equality check had an assignment of the variable output amount, the right hand side of that check used the same variable.\
According to the Solidity documentation, such a check constituted an instance of undefined behavior, and as such the behavior of that code was not specified and could change in future releases of Solidity.\
The recommendation was to rewrite the if statement such that it did not use and assign the same variable in an equality check and in general avoid undefined language usages.\
This is broadly related to the undefined behavior issues we discussed in number 179 of security pitfalls and best practices 201 module.
Another high severity finding from Trail of Bits audit of the DFX protocol was related to data validation, where certain functions returned raw values instead of converting them to numerical values that it used for its internal authority.\
Interchanging raw and human values would produce unwanted results and may have resulted in loss of funds for liquidity providers.\
The recommendation was to change the semantics of such functions to return the numeric balance instead of raw balance and in the long term, use unit tests and fuzzic to ensure that all calculations return the expected values. This is related to the broad aspect of data validation issues we discussed in number 169 of security pitfalls and best practices 201 modules.
This finding was a Trail of Bits audit of 0x protocol where it was a high severity finding related to specification, in which there was a specification called "mismatch for asset Proxy Owner timelock period" where the specification indicated that submitted transactions must pass a two week timelock before they were executed.\
The timelock period implementation did not enforce the two week period, but was instead configurable without any rain checks indicating that either the specification was outdated or that this was a serious flaw.\
The recommendation was to implement necessary rain checks to enforce the timelock period described in the specification, or otherwise correct the specification to match the intended behavior. This is related to the broad aspect of system specification of numbers 136 and 155, and clarity issues of 188 we discussed in security pitfalls and best practices 201 module.
Another high severity finding from Trail of Bits audit of the 0x protocol was again related to specification in which neither the 0x specification nor non-documentation stated clearly enough how fillable orders were determined.\
The recommendation was to define a proper procedure to determine if an order was available and detail it in the protocol specification and, if necessary, warn the users about potential constraints or others. This is related to the broad aspects of system specification of number 136 and 155, and clarity issues of 188 we discussed in the security pitfalls and best practices 201.
This finding was a Trail of Sigma Prime of Synthetix's EtherCollateral where it was a high severity finding related to data validation, in which there was improper enforcement of supply cap limitation.\
The openLoan() function only enforced that the supply cap was not reached before the loan was opened without considering the loan amount being opened. As a result, any account could create a node that exceeded the maximum amount that could be issued by the EtherCollateral contract.\
The recommendation was to add a require statement in the openLoan() function to prevent the total cap from being exceeded by the loan to be open.\
This is related to token handling in number 159 and broader aspects of data validation in 169 and accounting issues in 171 that we discussed in the security pitfalls and best practices 201 module.
Another high severity finding from Sigma Prime audit of the Synthetix's EtherCollateral was related to data validation and denial of service resulting from improper storage management.\
During opening loan accounts, when loans were opened, the associated account address got added to the accounts with open loans array regardless of whether it was already present in that array. Additionally, it was possible for a malicious attacker to create a denial of service condition exploiting the unbounded storage array in accounts synthetics.\
The recommendation was to consider changing the storeLoan() function to only push an account to the accounts with open loans array, if the loan to be stored was the first one for that particular account and to introduce a limit to the number of loans each account could have.\
This is related to the broad aspects of data validation in 169 accounting in 171 and denial of service issues in 176 that we discussed in security pitfalls and best practices 201 module.
This finding was a Trail of Sigma Prime of Infinigold where it was a high severity finding related to access control in which the transferFrom() function in the token implementation contract did not verify that the sender (that's the from address) is not blacklisted. As such, it was possible for a user to allow an account to spend a certain allowance regardless of their blacklisting status.
The recommendation was to use the notBlacklisted address modifier on the from address besides the msg.sender and two addresses. This is related to the aspect of access control in number 4 of security pitfalls and best practices 101 module and aspects of function modifiers in 141 and missing or incorrectly used modifiers and 150, 152 and broader aspects of access control in 172 that we discussed in security pitfalls and best practices 201 modules.
This finding was a Trail of Sigma Prime of Synthetix's Unipool where it was a high severity finding related to timing and ordering, in which staking before the initial notifyRewardAmount() led to disproportionate rewards.\
So, if a user successfully staked an amount of UNI tokens before the notifyRewardAmount() function was called for the first time, their initial user reward per token paid would be set to zero, the staker would be paid out funds greater than their share of SNX demands.\
The recommendation was to prevent state from being called before notified word amount was called for the first time. This is related to function invocation order in 145 and broader aspects of ordering in 178 and business logic issues in 191 that we discussed in security pitfalls and best practices 201 module.
Another high severity finding from Sigma Prime audit of the Synthetix's Unipool was related to error handling, in which an external call reverting would block minting.\
For context, the function notifyRewardAmount() would revert if block.timestamp was less than periodFinish(), but this function was called indirectly via Synthetix's mint() function, which meant that a reward would cause the external call to fail and thereby halt the mint process.\
The recommendation was to consider handling the case where the reward period had not elapsed without reverting. This is related to token handling in number 159, the broader aspect of error reporting issues in 175 that we discussed in security pitfalls and best practices 201 module.
This finding was a Trail of Sigma Prime of Chainlink where it was a high severity finding related to timing and denial of service, in which malicious users could DoS or hijack requests from changing contracts by replicating or Front-running legitimate requests.
This is made possible because requests could specify their own callbacks which could be abused by an attacker to frontrun or force the failure of legitimate requests.
The recommendation was to consider restricting arbitrary callbacks by making them localized to the requester themselves. This is related to transaction order dependence aspect discussed in 21 of security pitfalls and best practices 101 and denial of service aspect of 176 and external interaction issues of 180 we discussed in security pitfalls and best practices 201 modules.
This finding was an OpenZeppelin audit of Futureswap V2 where it was a high severity finding related to timing and denial of service, in which attackers could prevent users from performing an instant withdrawal from the wallet contract.
An attacker who observed a user's call to messageProcessor instantWithdraw() in Ethereum's mempool could get the Oracle message and Oracle signature parameters from the user's transaction, then submit their own transaction to instantWithdraw() using the same parameters but at a higher Gas Price to front run the user's transaction, but also carefully choosing the Gas Limit for the transaction such that the call would fail with an Out-of-Gas error at a certain point of the contract's flow.
The result would be that the attackers instead withdraw would fail, but the user interaction number would have been successfully reserved and as a result the user's transaction would revert because it would be attempting to use a user interaction number that was no longer valid.
The recommendation was to consider adding an access control mechanism to restrict who could submit Oracle messages on behalf of the user. This is related to the transaction order dependence aspect discussed in 21 of security pitfalls and best practices 101 module and access control aspects of 148 149 and 172 along with derivative service aspects of 176 that we discussed in security pitfalls and best practices 201.
This finding was an OpenZeppelin audit of Opyn Gamma where it was a high severity finding related to denial of service, in which Ether could get trapped in the protocol if a user sent more than the necessary ETH for a batch of actions in a specific context of the protocol.
The remaining Eth was not returned to the user and would get locked in the contract due to the lack of a withdrawal function.
The recommendation was to consider either returning all the remaining ETH to the user or creating a function that allowed the user to collect the remaining ETH. This is related to locked Ether in 29 of security pitfalls and best practices 101 module along with the broader aspects of numerical issues in 170 accounting issues in 171 and denial of service issues in 176 that we discussed in security pitfalls and best practices 201 module.
This finding was an OpenZeppelin audit of Endaoment where it was a high severity finding related to timing, in which a reentrancy vulnerability was present because of not following the checks effects interactions. For context, the finalized grant() function of the fund contract was setting the grantCompleteStorage variable after a token transfer and could therefore be used to conduct a reentrancy attack leading to the contract funds being traded.
The recommendation was to always follow the check effects interactions pattern where the contract state is modified before making any external call to other contracts and use reentrancy guards for such functions. This is related to OpenZeppelin's reentrancy guard library we discussed in 157 of Solidity 201 module re-entrance vulnerabilities in 13 of security pitfalls and best practices 101 module along with the broader aspects of timing issues in 177 that we discussed in security pitfalls and best practices 201.
This finding was an OpenZeppelin audit of Audius where it was a high severity finding related to auditing and logging in which no events were emitted while updating the governance registry and guardian addresses. For context, in the governance contract, the registry address and the guardian address are highly sensitive accounts which could be updated by calling setRegistryAddress() and transferGuardianship() respectively. However, these two functions did not emit any events. Because of that, stakers who monitor the Audius system would have to expect all transactions to detect and if any address they trusted was being replaced with an untrusted one instead of simply subscribing to events emitted.\
The recommendation therefore was to consider emitting events when these addresses were updated to make it more transparent and easier for clients to subscribe to the events when they want to keep track of the status of the system. This is related to missing events in 45 of security pitfalls and best practices one-on-one module along with broader aspects of auditing logging issues in 173 and principle of compromise recording in 201 that we discussed in security pitfalls and best practices 201.
Another high severity finding from OpenZeppelin audit of Audius was related to identification, in which the quorum requirement could be bypassed with sybil attack. For context, while the final vote on a proposal was determined via a token weighted vote, the quorum check in the evaluateProposalOutcome() function could be bypassed by splitting one's tokens over multiple accounts and voting with each of those accounts. Each of those sybil accounts increased the proposal's numbers variable which meant that anyone could make the quorum check pass.\
The recommendation was to consider measuring size by the percentage of existing tokens that have voted rather than the number of unique accounts that have loaded this is related to the broad aspects of token handling in 159 accounting in 171 and access control in 172 that we discussed in security pitfalls and best practices 201.
This finding was an OpenZeppelin audit of MCDEX Mai protocol where it was a high severity finding related to data validation in which votes could be duplicated. For context, the data verification mechanism used a commit-reveal-key to hide votes during the voting period, where the intention was to prevent voters from simply voting with the majority. However, the design allowed voters to blindly copy each other's submissions because votes were not cryptographically tied to the voter and so, undermined the objective of the commit-reveals.
The recommendation was to consider including the voter address within the commitment to prevent votes from being duplicated and also consider including the relevant timestamp price identifier and round id as well to limit the applicability and reusability of commitment. This is related to broad aspects of data validation in 169 accounting in 171 cryptography in 174 and business logic issues in 191 that we discussed in security pitfalls and best practices.