diff --git a/Changelog.md b/Changelog.md index 473282058042..f81b0d2a74c4 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,6 +1,8 @@ ### 0.9.0 (unreleased) Breaking changes: + * Disallow ``.pop()`` on arrays containing nested mappings. + * Disallow ``delete`` on types that contain nested mappings. * Inline Assembly: Consider functions, function parameters and return variables for shadowing checks. diff --git a/docs/security-considerations.rst b/docs/security-considerations.rst index 0fab056ff372..eeff23b58126 100644 --- a/docs/security-considerations.rst +++ b/docs/security-considerations.rst @@ -317,48 +317,12 @@ key-value data structure that does not keep track of the keys that were assigned a non-zero value. Because of that, cleaning a mapping without extra information about the written keys is not possible. If a ``mapping`` is used as the base type of a dynamic storage array, deleting -or popping the array will have no effect over the ``mapping`` elements. The -same happens, for example, if a ``mapping`` is used as the type of a member -field of a ``struct`` that is the base type of a dynamic storage array. The -``mapping`` is also ignored in assignments of structs or arrays containing a +or popping the array is not possible. +The same is true, for example, if a ``mapping`` is used as the type of a member +field of a ``struct`` that is the base type of a dynamic storage array. +It is also not possible to assign new values to structs or arrays containing a ``mapping``. -.. code-block:: solidity - - // SPDX-License-Identifier: GPL-3.0 - pragma solidity >=0.6.0 <0.9.0; - - contract Map { - mapping (uint => uint)[] array; - - function allocate(uint _newMaps) public { - for (uint i = 0; i < _newMaps; i++) - array.push(); - } - - function writeMap(uint _map, uint _key, uint _value) public { - array[_map][_key] = _value; - } - - function readMap(uint _map, uint _key) public view returns (uint) { - return array[_map][_key]; - } - - function eraseMaps() public { - delete array; - } - } - -Consider the example above and the following sequence of calls: ``allocate(10)``, -``writeMap(4, 128, 256)``. -At this point, calling ``readMap(4, 128)`` returns 256. -If we call ``eraseMaps``, the length of state variable ``array`` is zeroed, but -since its ``mapping`` elements cannot be zeroed, their information stays alive -in the contract's storage. -After deleting ``array``, calling ``allocate(5)`` allows us to access -``array[4]`` again, and calling ``readMap(4, 128)`` returns 256 even without -another call to ``writeMap``. - If your ``mapping`` information must be deleted, consider using a library similar to `iterable mapping `_, allowing you to traverse the keys and delete their values in the appropriate ``mapping``. diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 0f5ead34b7c2..f0ef5aae8f2b 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -1594,11 +1594,18 @@ bool TypeChecker::visit(UnaryOperation const& _operation) requireLValue(_operation.subExpression(), false); else _operation.subExpression().accept(*this); + Type const* subExprType = type(_operation.subExpression()); - Type const* t = type(_operation.subExpression())->unaryOperatorResult(op); - if (!t) + TypeResult result = subExprType->unaryOperatorResult(op); + Type const* t = result; + if (!result) { - string description = "Unary operator " + string(TokenTraits::toString(op)) + " cannot be applied to type " + subExprType->toString(); + string description = "Unary operator " + + string(TokenTraits::toString(op)) + + " cannot be applied to type " + + subExprType->toString() + + (result.message().empty() ? "" : (": " + result.message())); + if (modifying) // Cannot just report the error, ignore the unary operator, and continue, // because the sub-expression was already processed with requireLValue() @@ -2838,6 +2845,15 @@ bool TypeChecker::visit(MemberAccess const& _memberAccess) ); if ( + funType->kind() == FunctionType::Kind::ArrayPop && + exprType->containsNestedMapping() + ) + m_errorReporter.typeError( + 6298_error, + _memberAccess.location(), + "Storage arrays with nested mappings do not support .pop()." + ); + else if ( funType->kind() == FunctionType::Kind::ArrayPush && arguments.value().numArguments() != 0 && exprType->containsNestedMapping() diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp index e0928fe354fd..137afcba3d5a 100644 --- a/libsolidity/ast/Types.cpp +++ b/libsolidity/ast/Types.cpp @@ -1449,6 +1449,10 @@ TypeResult ReferenceType::unaryOperatorResult(Token _operator) const { if (_operator != Token::Delete) return nullptr; + + if (containsNestedMapping()) + return TypeResult::err("Contains a (possibly nested) mapping"); + // delete can be used on everything except calldata references or storage pointers // (storage references are ok) switch (location()) diff --git a/test/libsolidity/semanticTests/storage/mappings_array2d_pop_delete.sol b/test/libsolidity/semanticTests/storage/mappings_array2d_pop_delete.sol deleted file mode 100644 index f2def79bc8e7..000000000000 --- a/test/libsolidity/semanticTests/storage/mappings_array2d_pop_delete.sol +++ /dev/null @@ -1,41 +0,0 @@ -contract C { - mapping (uint => uint)[][] a; - - function n1(uint key, uint value) public { - a.push(); - mapping (uint => uint)[] storage b = a[a.length - 1]; - b.push(); - b[b.length - 1][key] = value; - } - - function n2() public { - a.push(); - mapping (uint => uint)[] storage b = a[a.length - 1]; - b.push(); - } - - function map(uint key) public view returns (uint) { - mapping (uint => uint)[] storage b = a[a.length - 1]; - return b[b.length - 1][key]; - } - - function p() public { - a.pop(); - } - - function d() public returns (uint) { - delete a; - return a.length; - } -} -// ==== -// compileViaYul: also -// ---- -// n1(uint256,uint256): 42, 64 -> -// map(uint256): 42 -> 64 -// p() -> -// n2() -> -// map(uint256): 42 -> 64 -// d() -> 0 -// n2() -> -// map(uint256): 42 -> 64 diff --git a/test/libsolidity/semanticTests/storage/mappings_array_pop_delete.sol b/test/libsolidity/semanticTests/storage/mappings_array_pop_delete.sol deleted file mode 100644 index 5078103b81c9..000000000000 --- a/test/libsolidity/semanticTests/storage/mappings_array_pop_delete.sol +++ /dev/null @@ -1,36 +0,0 @@ -contract C { - mapping (uint => uint)[] a; - - function n1(uint key, uint value) public { - a.push(); - a[a.length - 1][key] = value; - } - - function n2() public { - a.push(); - } - - function map(uint key) public view returns (uint) { - return a[a.length - 1][key]; - } - - function p() public { - a.pop(); - } - - function d() public returns (uint) { - delete a; - return a.length; - } -} -// ==== -// compileViaYul: also -// ---- -// n1(uint256,uint256): 42, 64 -> -// map(uint256): 42 -> 64 -// p() -> -// n2() -> -// map(uint256): 42 -> 64 -// d() -> 0 -// n2() -> -// map(uint256): 42 -> 64 diff --git a/test/libsolidity/semanticTests/structs/delete_struct.sol b/test/libsolidity/semanticTests/structs/delete_struct.sol index fda4e20e126e..265fa2fbbde6 100644 --- a/test/libsolidity/semanticTests/structs/delete_struct.sol +++ b/test/libsolidity/semanticTests/structs/delete_struct.sol @@ -2,23 +2,17 @@ contract test { struct topStruct { nestedStruct nstr; uint topValue; - mapping (uint => uint) topMapping; } uint toDelete; topStruct str; struct nestedStruct { uint nestedValue; - mapping (uint => bool) nestedMapping; } constructor() { toDelete = 5; str.topValue = 1; - str.topMapping[0] = 1; - str.topMapping[1] = 2; str.nstr.nestedValue = 2; - str.nstr.nestedMapping[0] = true; - str.nstr.nestedMapping[1] = false; delete str; delete toDelete; } @@ -31,12 +25,6 @@ contract test { function getNestedValue() public returns(uint nestedValue){ nestedValue = str.nstr.nestedValue; } - function getTopMapping(uint index) public returns(uint ret) { - ret = str.topMapping[index]; - } - function getNestedMapping(uint index) public returns(bool ret) { - return str.nstr.nestedMapping[index]; - } } // ==== // compileViaYul: also @@ -44,7 +32,3 @@ contract test { // getToDelete() -> 0 // getTopValue() -> 0 // getNestedValue() -> 0 #mapping values should be the same# -// getTopMapping(uint256): 0 -> 1 -// getTopMapping(uint256): 1 -> 2 -// getNestedMapping(uint256): 0 -> true -// getNestedMapping(uint256): 1 -> false diff --git a/test/libsolidity/syntaxTests/array/pop/storage_with_mapping_pop.sol b/test/libsolidity/syntaxTests/array/pop/storage_with_mapping_pop.sol index d798aee13554..6844dd6dc54f 100644 --- a/test/libsolidity/syntaxTests/array/pop/storage_with_mapping_pop.sol +++ b/test/libsolidity/syntaxTests/array/pop/storage_with_mapping_pop.sol @@ -6,3 +6,4 @@ contract C { } } // ---- +// TypeError 6298: (109-118): Storage arrays with nested mappings do not support .pop(). diff --git a/test/libsolidity/syntaxTests/negation.sol b/test/libsolidity/syntaxTests/negation.sol index f79920954e61..8e6782c61325 100644 --- a/test/libsolidity/syntaxTests/negation.sol +++ b/test/libsolidity/syntaxTests/negation.sol @@ -6,4 +6,4 @@ contract test { } } // ---- -// TypeError 4907: (97-99): Unary operator - cannot be applied to type uint256 +// TypeError 4907: (97-99): Unary operator - cannot be applied to type uint256: Unary negation is only allowed for signed integers. diff --git a/test/libsolidity/syntaxTests/types/mapping/assign_storage_struct_with_mapping.sol b/test/libsolidity/syntaxTests/types/mapping/assign_storage_struct_with_mapping.sol new file mode 100644 index 000000000000..00666b19e355 --- /dev/null +++ b/test/libsolidity/syntaxTests/types/mapping/assign_storage_struct_with_mapping.sol @@ -0,0 +1,15 @@ +contract D { + struct Test { + mapping(address => uint) balances; + } + + Test test; + + constructor() + { + test = Test(); + } +} +// ---- +// TypeError 9214: (102-106): Types in storage containing (nested) mappings cannot be assigned to. +// TypeError 9515: (109-115): Struct containing a (nested) mapping cannot be constructed. diff --git a/test/libsolidity/syntaxTests/types/mapping/delete_storage_struct_with_mapping.sol b/test/libsolidity/syntaxTests/types/mapping/delete_storage_struct_with_mapping.sol new file mode 100644 index 000000000000..af131ee2db5c --- /dev/null +++ b/test/libsolidity/syntaxTests/types/mapping/delete_storage_struct_with_mapping.sol @@ -0,0 +1,14 @@ +contract D { + struct Test { + mapping(address => uint) balances; + } + + Test test; + + constructor() + { + delete test; + } +} +// ---- +// TypeError 9767: (102-113): Unary operator delete cannot be applied to type struct D.Test storage ref: Contains a (possibly nested) mapping