Platform: Code4rena
Start Date: 29/04/2022
Pot Size: $22,000 USDC
Total HM: 6
Participants: 40
Period: 3 days
Judge: Justin Goro
Total Solo HM: 2
Id: 114
League: ETH
Rank: 22/40
Findings: 1
Award: $110.78
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0x4non, 0xDjango, 0xf15ers, 0xkatana, 242, Dravee, GimelSec, MaratCerby, Tadashi, TrungOre, WatchPug, defsec, fatherOfBlocks, gzeon, hake, horsefacts, joestakey, miguelmtzinf, pauliax, pedroais, peritoflores, rotcivegaf, simon135, slywaters, tabish, throttle, z3s
110.7777 USDC - $110.78
There are multiple variables that can be immutable and their use can be "optimized".
First of all, aToken
, rewardsController
and poolAddressesProviderRegistry
could be immutable: there are no setters for them and all of them are proxies: in case of a change, the implementation address will change, not the proxy address.
In the case of the poolAddressesProviderRegistry
:
poolAddressesProviderRegistry
is only needed to fetch the pool address, but there is no need to store this address, only the address of the pool (and again, is a proxy). So, there are 2 options:
- Instead of passing thepoolAddressesProviderRegistry
, you can pass the poolProviderAddress
directly and store its address as immutable.
- You can pass the poolAddressesProviderRegistry
, but only store the poolProviderAddress
(making the call at the constructor and not in every call).
So, in supplyTokenTo()
and redeemToken()
there is a call to _pool()
which calls _poolProvider()
and getPool()
--> 4 calls + 1 SLOAD in total. This can be done with 0 calls and 0 SLOAD just storing the pool address at the constructor.
In the case of aToken
:
The aToken
address does not change (proxy) and its underlying wont never change. In supplyTokenTo()
and redeemToken()
there is a call to _tokenAddress()
that can be skipped (1 SLOAD + 1 CALL) just storing the underlying address as immutable (doing it at the constructor; the aToken
address can also be stored as immutable, just for transparency).
Gas comparison reducing the cost as much as possible: https://gist.github.com/miguelmtzinf/fb6d5266fb2fd3d057028b84fc0f66e1 (also provides files with changes)
These changes reduces the gas cost across the board, and makes the most important functions way cheaper:
supplyTokenTo
: 40k lessredeemToken
: 40k lessBonus track (not applied in the gist):
balanceOfToken()
can be view.(inhereted ERC20)
, `(inherited ERC20).transferERC20()
at L225 you are not using the already built _requireNotAToken()
internal function. This function could have a more generic revert message and be used also here.#0 - PierrickGT
2022-05-04T19:45:24Z
First of all, aToken, rewardsController and poolAddressesProviderRegistry could be immutable
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/1
poolAddressesProviderRegistry is only needed to fetch the pool address
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/17
The aToken address does not change (proxy) and its underlying wont never change.
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/19
The function balanceOfToken() can be view.
Can't because we inherit from the yield source interface.
Typo in L154. Instead of (inhereted ERC20), `(inherited ERC20).
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/22
In transferERC20() at L225 you are not using the already built _requireNotAToken() internal function.
Duplicate of https://github.com/code-423n4/2022-04-pooltogether-findings/issues/4