GalloDaSballo
commented
2 years ago Condition in receive function can be bypassed with self-destruct of another contract
My conclusion from this is to add a sweep for ETH, that said who's going to give us free money? -> entreprenerd.eth
Re-entrancy guard upgradeable contract is not initialized
Agree that we should and agree with severity as it's just a gas cost
Usage of deprecated function safeApprove
I'm pretty sure we're using it the correct way (one time, from 0 to X)
Alert developers that OpenZeppelin contract cannot be bumped to 4.x
Agree with the heads up and believe this is the correct severity for the other "OZ Initializer Vulnerability hehe XD"
Condition in
receivefunction can be bypassed with self-destruct of another contractDetails: The logic in L436-L438 implies that the contract should only receive ether if
isClaimingBribesistrue. However, this check can be bypassed by deploying a contract (say, Attacker) and setting up the address of MyStrategy contract as the destination of aselfdestructin the Attacker contract — for more information and otherway to bypass the require of L437, see this link.Impact: Informational (could possibly break internal calculations of the protocol though)
Re-entrancy guard upgradeable contract is not initialized
Details: As stated in OpenZeppelin docs, “when writing an initializer, you need to take special care to manually call the initializers of all parent contracts”. However, the initializer of
ReentrancyGuardUpgradeableis not called.Mitigation: Ensure that all necessary functions are inherited from the upgradeable contracts.
Impact: Code QA
TODOs are left in comments
Details: In L284 and L422 of MyStrategy.sol there are comments with TODOs. These should be resolved and removed from the code before deployment.
Mitigation: Check the TODOs and fix/remove them.
Impact: Code QA
Usage of deprecated function safeApprove
Details: In L65-68 of MyStrategy.sol the function
safeApprovefrom OpenZeppelin contracts are used, however these functions have been deprecated according to OpenZeppelin 3.x docs (note that MyStrategy.sol correctly use OpenZeppelin 3.4.0).Impact: Code QA
Alert developers that OpenZeppelin contract cannot be bumped to 4.x
Note to judges: I think this issue is out-of-scope, but worthy to inform anyway 🙂
Details: According to brownie config file, the contract MyStrategy.sol imports version 3.4.0 of OpenZeppelin’s SafeMath, and this is the recommend version to use with Solidity 0.6.12.
Unware developers, however, may want to bump OpenZeppelin version to the lastest one, and running
brownie compilewill compile the contract without errors (at least for 4.6.0). However, as alerted by the comments in L6-8, recent versions of SafeMath should only be used with Solidity 0.8 or later, because it relies on the compiler's built in overflow checks. This implies that checks of overflow/underflow will not be used, and this could be further exploited in other attacks.This could also be particularly dangerous in the scenario wherein a developer does this bumping while writing his own MyStrategy contract, since he will probably use the SafeMath functions assuming that underflow/overflow checks are being used in his code.
Mitigation: Consider adding a comment in brownie config file alerting the users that OpenZeppelin version should not be bumped.
Impact: Informational (probably out-of-scope)