Open code423n4 opened 2 years ago
call setMineter with zero address wait for the timelock
This alone will trigger awareness that something malicious is happening and since timelock is there people have time to get out.
the second POC is valid
removed all secondary time locks in the contract and only using the primary timelock that will be behind the owner.
Lines of code
https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/governance/GolomToken.sol#L58-L72 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/core/GolomTrader.sol#L444-L457 https://github.com/code-423n4/2022-07-golom/blob/7bbb55fca61e6bae29e57133c1e45806cbb17aa4/contracts/rewards/RewardDistributor.sol#L298-L311
Vulnerability details
Impact
MED - Function could be impacted
As the timelock does not work as supposed to work, the owner of the contract can bypass timelock.
GolomToken
:setMinter
,executeSetMinter
GolomTrader
:setDistributor
,executeSetDistributor
RewardDistributor
:addVoteEscrow
,executeAddVoteEscrow
Proof of Concept
The first poc shows to bypass timelock for
GolomTrader::setDistributor
. The same logic applies for theRewardDistributor::addVoteEscrow
.setDistributor
was called once in the beforeEach block to set the initial distributor. For this exploit to work, thesetDistributor
should be called only once. IfsetDistributor
was called more than once, one can set the distributor to zero address (with timelock like in theGolomToken
case, then set to a new distributor after that)executeSetDistributor
setDistributor
setDistributor
is not called multiple times in row, the owner can keep setting distributor without timelock.A little bit different variation of timelock bypass was found in the
GolomToken
. Although the owner should wait for the timelock to set the minter to zero address, but after that, the owner can set to the new minter without waiting for a timelock. Since the meaning of timelock is to let people know the new minter's implementation, if the owner can bypass that, the timelock is almost meaningless. The exploitation steps: the second proof of conceptsetMineter
with zero addressexecuteSetMineter
to set the minter to zero addresssetMineter
with any address and callexecuteSetMinter
without waiting for the timelockThe owner can call
executeSetdistributor
even though there is nopendingDistributor
set before. Also,setDistributor
sets the new distributor without timelock when the existing distributor's address is zero.Tools Used
None
Recommended Mitigation Steps
To mitigate, execute functions can check whether pendingDistributor is not zero. It will ensure that the setters are called before executing them, as well as prevent to set to zero addresses.