Platform: Code4rena
Start Date: 26/07/2022
Pot Size: $75,000 USDC
Total HM: 29
Participants: 179
Period: 6 days
Judge: LSDan
Total Solo HM: 6
Id: 148
League: ETH
Rank: 18/179
Findings: 7
Award: $676.85
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: GimelSec
Also found by: 0x52, 0xA5DF, 0xSky, 0xsanson, Bahurum, CertoraInc, GalloDaSballo, JohnSmith, Lambda, MEP, Twpony, arcoun, berndartmueller, cryptphi, hansfriese, kenzo, kyteg, panprog, rajatbeladiya, scaraven, simon135, zzzitron
26.7695 USDC - $26.77
In VoteEscrowDelegation.sol in the function delegate when nCheckpoint = 0, which is the case for every account that has not delegated yet, the function _writeCheckpoint is called. However, this function computes (nCheckpoints - 1) L101 which thus causes an underflow. Contracts are compiled in 8.11.0 so underflow reverts, and no one is then able to delegate his voting power.
Suggested fix: the function _writeCheckpoint should check if nCheckpoints is zero and behave accordingly.
#0 - KenzoAgada
2022-08-02T09:05:28Z
Duplicate of #630
26.7695 USDC - $26.77
The mapping delegates in VoteEscrowDelegation can be misleading (it is not updated when a delegation is removed). If someone is relying on this value (not the case here, because this mapping is never used in the contracts (?), but we can assume that it has / will have / should have a purpose).
Suggested fix: update the mapping when a delegation is removed.
#0 - KenzoAgada
2022-08-02T12:11:52Z
Duplicate of #169
🌟 Selected for report: CertoraInc
Also found by: 0xA5DF, 0xsanson, Bahurum, GalloDaSballo, MEP, TrungOre, carlitox477, cryptphi, kenzo
In VoteEscrowDelegation.sol L242, the function removeDelegation is called externally (with the syntax this.), but that external call changes the msg.sender. So in the function removeDelegation, msg.sender will be the contract itself, not the user. But this function requires that the owner of the NFT is msg.sender, that will always be false for NFTs owned by the user.
Sggested fix: call removeDelegation internally (so make this function public instead of external).
#0 - KenzoAgada
2022-08-02T05:52:03Z
Duplicate of #377
93.2805 USDC - $93.28
When a delegation is created (with the function delegate, the ID of its NFT (tokenId) is added in the array checkpoints[toTokenId][nCheckpoints - 1]; where toTokenId is the ID of NFT the user delegates its NFT to.
When an user removes the delegation of his NFT (with the function removeDelegation L210), tokenId is removed from checkpoints[tokenId][nCheckpoints - 1] istead of checkpoints[toTokenId][nCheckpoints - 1].
toTokenId can be retrieved in removeDelegation through the mapping delegates, but be aware that this mapping is not accurate (reported as a Medium).
#0 - KenzoAgada
2022-08-02T08:22:58Z
Duplicate of #751
🌟 Selected for report: cloudjunky
Also found by: 0x1f8b, 0x4non, 0x52, 0xDjango, 0xHarry, 0xNazgul, 0xNineDec, 0xf15ers, 0xsanson, 0xsolstars, 8olidity, Bnke0x0, CertoraInc, Chom, Deivitto, Dravee, GalloDaSballo, GimelSec, IllIllI, Jmaxmanblue, JohnSmith, Jujic, Kenshin, Krow10, Lambda, MEP, Noah3o6, RedOneN, Ruhum, StErMi, StyxRave, TomJ, Treasure-Seeker, TrungOre, _Adam, __141345__, arcoun, asutorufos, bardamu, bearonbike, bin2chen, brgltd, bulej93, c3phas, cRat1st0s, carlitox477, cccz, codexploder, cryptonue, cryptphi, cthulhu_cult, dharma09, dipp, djxploit, durianSausage, ellahi, giovannidisiena, hansfriese, horsefacts, hyh, immeas, indijanc, jayjonah8, jayphbee, joestakey, kenzo, kyteg, ladboy233, minhquanym, navinavu, obront, oyc_109, peritoflores, rbserver, reassor, rokinot, rotcivegaf, saian, scaraven, shenwilly, simon135, sseefried, teddav, zzzitron
0.0037 USDC - $0.00
The payEther() of GolomTrader.sol uses tranfer() to send ether. An annoying person could create an appealing offer from an address that does not accept incoming eth (typically a solidity contract without fallback() nor receive()). Thus, fooled users will send transactions that will revert.
One could use send() instead, but this would prevent multisig from using this protocol, because of the lack of gas (capped to 23000, contrary to call()).
Suggested fix: use .call with a value, and do not revert in case of failure.
#0 - KenzoAgada
2022-08-03T14:03:03Z
Duplicate of #343
A DoS is possible in VoteEscrowDelegation.sol, in the function delegate
L99 : require(_delegatedTokenIds.length < 500, 'VVDelegation: Cannot stake more');
But delegating an NFT is cheap: it is possible to create_lock with as low as 1 wei of token and then to delegate the NFT. Thus an attacker could delegate 500 NFT paying pretty much nothing but gas fees. Then, it would not be possible for anyone to use the functiondelegate with the same target NFT.
#0 - 0xA5DF
2022-08-11T09:32:43Z
Warden has a point, however the attacker would have to spend a significant amount of gas (at least 1000 function calls), while this would only disable 1 token from being delegated to. The owner of the token attacked can simply create a new token that people would be delegating to instead.
Also, there's no recommended mitigation (increasing the array size doesn't make sense, maybe have a minimum golom tokens to lock?).
#1 - zeroexdead
2022-08-15T14:17:03Z
Duplicate of #707
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x4non, 0x52, 0xA5DF, 0xDjango, 0xLovesleep, 0xNazgul, 0xNineDec, 0xSmartContract, 0xackermann, 0xc0ffEE, 0xf15ers, 0xmatt, 0xsanson, 0xsolstars, 8olidity, AuditsAreUS, Bahurum, Bnke0x0, CRYP70, CertoraInc, Ch_301, Chom, CryptoMartian, Deivitto, DevABDee, Dravee, ElKu, Franfran, Funen, GalloDaSballo, GimelSec, GiveMeTestEther, Green, JC, Jmaxmanblue, JohnSmith, Jujic, Junnon, Kenshin, Krow10, Kumpa, Lambda, MEP, Maxime, MiloTruck, Mohandes, NoamYakov, Picodes, RedOneN, Rohan16, Rolezn, Ruhum, RustyRabbit, Sm4rty, Soosh, StErMi, StyxRave, Tadashi, TomJ, Treasure-Seeker, TrungOre, Waze, _Adam, __141345__, ajtra, ak1, apostle0x01, arcoun, asutorufos, async, benbaessler, berndartmueller, bin2chen, brgltd, c3phas, cRat1st0s, carlitox477, chatch, codetilda, codexploder, cryptonue, cryptphi, csanuragjain, cthulhu_cult, delfin454000, dipp, dirk_y, djxploit, ellahi, exd0tpy, fatherOfBlocks, giovannidisiena, hansfriese, horsefacts, hyh, idkwhatimdoing, indijanc, jayfromthe13th, jayphbee, joestakey, kenzo, kyteg, lucacez, luckypanda, mics, minhquanym, obront, oyc_109, pedr02b2, rajatbeladiya, rbserver, reassor, robee, rokinot, rotcivegaf, sach1r0, saian, saneryee, sashik_eth, scaraven, shenwilly, simon135, sseefried, supernova, teddav, ych18, zuhaibmohd, zzzitron
35.1687 USDC - $35.17
GolomTrader.sol, L177 should be removed to match the purpose of the function.GolomTrader.sol L45, the variable WETH should be set constant.GolomTrader, the functions fillAsk, fillBid, cancelOrder and fillCriteriaBid should be set external instead of public.RewardDistributor.sol, an interface ERC20 is defined, with functions that usual ERC20 token do not implement (balanceOfNFTAt L26, that is never used in the code, and deposit). Consider creating a dinstinct interfaces for the WETH token (that implements the function deposit and that inherits from a classic IERC20 interface), and the rewardToken can implement a classic IERC20 interface.RewardDistributor, consider using 1 days instad of defining a constant secsInDay.IVoteEscrow defined L12 of VoteEscrowDelegation.sol is never used.VoteEscrowDelegation.sol L68, template notice is left in commentaryVoteEscrowDelegation.sol misspelling L227VoteEscrowCore.sol misspellings L526 & L688VoteEscrowCore.sol L1108, commentary about Vyper not deleted when copy pasted the codeVoteEscrowCore.sol L8, the author is not Curve Finance, their contract is written in Vyper. Should precise that this contract is inspired by Curve Finance's Voting Escrow contract.