Platform: Code4rena
Start Date: 07/10/2022
Pot Size: $50,000 USDC
Total HM: 4
Participants: 62
Period: 5 days
Judge: 0xean
Total Solo HM: 2
Id: 169
League: ETH
Rank: 2/62
Findings: 2
Award: $5,676.97
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: ladboy233
Also found by: csanuragjain
5626.6867 USDC - $5,626.69
https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/gateway/L2GraphTokenGateway.sol#L87 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/l2/token/L2GraphToken.sol#L48 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/L1GraphTokenGateway.sol#L99 https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/BridgeEscrow.sol#L20
initialize function in L2GraphToken.sol, BridgeEscrow.sol, L2GraphTokenGateway.sol, L1GraphTokenGateway.sol
can be invoked multiple times from the implementation contract.
this means a compromised implementation can reinitialize the contract above and
become the owner to complete the privilege escalation then drain the user's fund.
Usually in Upgradeable contract, a initialize function is protected by the modifier
initializer
to make sure the contract can only be initialized once.
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
The implementation contract is compromised,
The attacker reinitialize the BridgeEscrow contract
function initialize(address _controller) external onlyImpl { Managed._initialize(_controller); }
the onlyGovernor modifier's result depends on the controller because
function _onlyGovernor() internal view { require(msg.sender == controller.getGovernor(), "Caller must be Controller governor"); }
The attacker have the governor access to the BridgeEscrow,
The attack can call the approve function to approve malicious contract
function approveAll(address _spender) external onlyGovernor { graphToken().approve(_spender, type(uint256).max); }
Manual Review
We recommend the project use the modifier
initializer
to protect the initialize function from being reinitiated
function initialize(address _owner) external onlyImpl initializer {
#0 - 0xean
2022-10-16T13:58:37Z
Sponsor - please also review #72 which is a duplicate but has a slightly different impact statement to the same underlying issue.
#1 - pcarranzav
2022-10-18T14:41:07Z
The impact from #72 was discussed with a warden and I consider it valid and worth fixing.
The impact from this one requires the implementation to be compromised in a way that allows calling initialize(). Since all implementations use GraphUpgradeable, I don't see how that's possible without also compromising the GraphProxyAdmin and therefore compromising the governor, which would mean much bigger problems.
I still consider this risky enough that a Medium severity is reasonable, both in #72 and this one.
#2 - trust1995
2022-10-18T21:10:37Z
Exploit of this bug requires ProxyAdmin compromise, and if attacker has that then there are many ways to severely damage the proxy. It's a good observation and should be fixed, but it seems to be a best practice issue.
#3 - pcarranzav
2022-10-21T17:09:36Z
#4 - 0xean
2022-10-21T21:36:57Z
@trust1995 - makes reasonable points, but I think its still correct to award this as M.
#5 - trust1995
2022-10-25T06:57:25Z
Would like to restate my strong opinion that this is not a vulnerability but a best practice issue. The only way to abuse the lack of "initializer" modifier is with ProxyAdmin calling the IMPL contract. But it is known ProxyAdmin can just change the IMPL contract to any it desires. The risk of ProxyAdmin being compromised is a generic centralization issue, applicable to every upgradeable structure and is far too much of a known risk to qualify as M.
As further evidence, 5 QA reports categorized the find as Low / QA. They also should be upgraded to M, or this report rightfully moved to QA / Low.
🌟 Selected for report: 0xSmartContract
Also found by: 0x1f8b, 0x4non, 0xNazgul, Bnke0x0, Chom, IllIllI, Josiah, Rahoz, RaymondFam, Trust, Waze, ajtra, bobirichman, brgltd, bulej93, c3phas, cccz, chrisdior4, delfin454000, fatherOfBlocks, gogo, ladboy233, mcwildy, mics, nicobevi, oyc_109, rbserver, rotcivegaf, zzzitron
50.2765 USDC - $50.28
token.transferFrom(from, escrow, _amount);
The contract GraphTokenUpgradeable.sol inherits
ERC20BurnableUpgradeable
contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgradeable {
however, the function
__ERC20_init
is called,
we recommend the project change the call to
__ERC20Burnable_init
The openzeppelin version used is outdated.
"@openzeppelin/contracts": "^3.4.1", "@openzeppelin/contracts-upgradeable": "3.4.2",
The updated solidity version is 0.8.17
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma abicoder v2;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity >=0.6.12 <0.8.0;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity >=0.6.12 <0.8.0;
pragma solidity ^0.7.6;
pragma solidity >=0.6.12 <0.8.0;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
pragma solidity ^0.7.6;
User will not know which steps goes wrong when transaction reverted.
require(
require(success);
require(success);
require(success);
require(success);
#0 - pcarranzav
2022-10-19T17:40:52Z
Use __ERC20Burnable_init instead of __ERC20_init in GraphTokenUpgradeable.sol
I checked and __ERC20Burnable_init does not call __ERC20_init afaict, so we do need to call __ERC20_init.