Platform: Code4rena
Start Date: 03/05/2022
Pot Size: $75,000 USDC
Total HM: 6
Participants: 55
Period: 7 days
Judge: Albert Chon
Total Solo HM: 2
Id: 116
League: COSMOS
Rank: 8/55
Findings: 4
Award: $1,836.27
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: p_crypt0
Also found by: 0x1337, GermanKuber, IllIllI, WatchPug, csanuragjain, danb, dirk_y, kirk-baird
Someone with access to admin keys could rug pull all funds
The gravity.sol contract should work as an escrow to mint equivalent tokens in the cosmos chain. This is maintained by a system of validators. The possible decentralization of the validators is ruined by the withdraw withdrawERC20 function that allows a single address with the admin role to take all tokens.
This adds a single point of failure to the contract since these admin keys could take all the money without any voting by the validators. If the keys are lost an attacker could use them to get all user funds. Also it lessens the trust in the contracts since the admin has to be trusted.
Remove this function. The only way to take tokens out should be by vote of a portion of the validators.
#0 - mlukanova
2022-05-10T14:34:23Z
Duplicate of #14
Loss of 1 to 1 ratio with fee on transfer tokens
Some tokens like USDT have a fee on transfer that can be activated. If such a token is used then wrong amounts will be minted on the other side. As we can see in the sendToCosmos function in gravity.sol the emitted event (which will be the cause of the mint on the cosmos side) will be emitted with the transfer amount.
If the token has a fee on transfer the real transfer amount could be less. For example if the fee is 0.1 % and 1000 USDT is sent the contract will receive 999 USDT but the event will emit 1000 USDT that will be minted on the cosmos side. If this is repeated the difference between the amount escrowed in the contract and the amount minted on cosmos will grow larger and larger until not all tokens can be redeemed.
Check the balance difference after transference and emit an amount equal to the balance difference
#0 - mlukanova
2022-05-10T14:47:30Z
Duplicate of #3
🌟 Selected for report: IllIllI
Also found by: 0x1337, 0x1f8b, 0xDjango, 0xkatana, AmitN, CertoraInc, Dravee, Funen, GermanKuber, GimelSec, Hawkeye, JC, MaratCerby, WatchPug, Waze, broccolirob, cccz, ch13fd357r0y3r, cryptphi, danb, defsec, delfin454000, dipp, dirk_y, ellahi, gzeon, hake, hubble, ilan, jah, jayjonah8, kebabsec, kirk-baird, m9800, orion, oyc_109, robee, shenwilly, simon135, sorrynotsorry
188.2942 USDC - $188.29
MAX_UINT would spend less gas if it is a constant (tests on remix said a difference of 5600).
🌟 Selected for report: GermanKuber
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xNazgul, 0xf15ers, 0xkatana, AlleyCat, CertoraInc, Dravee, Funen, GimelSec, IllIllI, JC, MaratCerby, WatchPug, Waze, defsec, delfin454000, ellahi, gzeon, hake, hansfriese, ilan, jonatascm, nahnah, oyc_109, peritoflores, rfa, robee, simon135, slywaters, sorrynotsorry
643.0392 USDC - $643.04
In the sendToCosmos() function it is not validated that _amount != 0, therefore the state_lastEventNonce could be made to grow only by spending gas. If they go up to type(uint256).max could it cause an overflow and DoS system wide?
#0 - V-Staykov
2022-05-10T13:23:00Z
Marked it with "disagree with severity" because this is not a gas optimization issue. It seems to be low/mid finding. It is indeed a valid issue, but mitigating it with just checking if the amount is not zero doesn't seem good, since an attack can then be made with _amount= 1e-18 lets say and still be cheap enough.
#1 - V-Staykov
2022-05-11T14:07:12Z
Duplicate of #85