Skip to content

Commit cdaed71

Browse files
authored
AragonApp: fix possibly uninitialized bytes array when checking permissions (#448)
1 parent 02beaaa commit cdaed71

File tree

3 files changed

+18
-11
lines changed

3 files changed

+18
-11
lines changed

contracts/acl/ACL.sol

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,13 +242,16 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
242242
* @return boolean indicating whether the ACL allows the role or not
243243
*/
244244
function hasPermission(address _who, address _where, bytes32 _what, bytes memory _how) public view returns (bool) {
245+
// Force cast the bytes array into a uint256[], by overwriting its length
246+
// Note that the uint256[] doesn't need to be initialized as we immediately overwrite it
247+
// with _how and a new length, and _how becomes invalid from this point forward
245248
uint256[] memory how;
246249
uint256 intsLength = _how.length / 32;
247250
assembly {
248-
how := _how // forced casting
251+
how := _how
249252
mstore(how, intsLength)
250253
}
251-
// _how is invalid from this point fwd
254+
252255
return hasPermission(_who, _where, _what, how);
253256
}
254257

contracts/apps/AragonApp.sol

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,14 @@ contract AragonApp is AppStorage, Autopetrified, VaultRecoverable, EVMScriptRunn
4747
return false;
4848
}
4949

50-
bytes memory how; // no need to init memory as it is never used
51-
if (_params.length > 0) {
52-
uint256 byteLength = _params.length * 32;
53-
assembly {
54-
how := _params // forced casting
55-
mstore(how, byteLength)
56-
}
50+
// Force cast the uint256[] into a bytes array, by overwriting its length
51+
// Note that the bytes array doesn't need to be initialized as we immediately overwrite it
52+
// with _params and a new length, and _params becomes invalid from this point forward
53+
bytes memory how;
54+
uint256 byteLength = _params.length * 32;
55+
assembly {
56+
how := _params
57+
mstore(how, byteLength)
5758
}
5859
return linkedKernel.hasPermission(_sender, address(this), _role, how);
5960
}

contracts/kernel/Kernel.sol

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -228,13 +228,16 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant
228228
}
229229

230230
modifier auth(bytes32 _role, uint256[] memory params) {
231+
// Force cast the uint256[] into a bytes array, by overwriting its length
232+
// Note that the bytes array doesn't need to be initialized as we immediately overwrite it
233+
// with params and a new length, and params becomes invalid from this point forward
231234
bytes memory how;
232235
uint256 byteLength = params.length * 32;
233236
assembly {
234-
how := params // forced casting
237+
how := params
235238
mstore(how, byteLength)
236239
}
237-
// Params is invalid from this point fwd
240+
238241
require(hasPermission(msg.sender, address(this), _role, how), ERROR_AUTH_FAILED);
239242
_;
240243
}

0 commit comments

Comments
 (0)