Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The SafeMath.div function needlessly allocates memory #2413

Closed
nlordell opened this issue Nov 26, 2020 · 4 comments · Fixed by #2462
Closed

The SafeMath.div function needlessly allocates memory #2413

nlordell opened this issue Nov 26, 2020 · 4 comments · Fixed by #2462
Labels
bug contracts Smart contract code.

Comments

@nlordell
Copy link
Contributor

The SafeMath.div function needlessly allocates memory.

💻 Environment

All environments.

📝 Details

Calling SafeMath.div(uint256, uint256) allocates memory in order to pass a revert reason to SafeMath.div(uint256, uint256, string memory), even if the operation would not revert. This causes an addition 64-byte allocation per call for the revert reason string length || paddedContents.

While in it of itself, this is a minor issue, it is worth noting that the other SafeMath methods do not have the same problem, and that in a loop, the gas cost of SafeMath.div grows quadratically (because of memory costs) instead of linearly with the number of iterations as one would naturally expect.

🔢 Code to reproduce bug

The easiest way to reproduce this is to use the following code in Remix and calling doThings with varying lengths of things input:

// SPDX-License-Identifier: CC0-1.0
pragma solidity ^0.6.0;

import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.2.0/contracts/math/SafeMath.sol";

contract SafeMathDivMemoryTest {
    using SafeMath for uint256;
    
    function doManyThings(uint256[] calldata things) external pure returns (uint256[] memory result, uint256 mem) {
        result = new uint256[](things.length);
        
        assembly {
            mem := mload(0x40)
        }
        for (uint256 i = 0; i < things.length; i++) {
            result[i] = doThing(things[i]);
        }
        assembly {
            mem := sub(mload(0x40), mem)
        }
    }
    
    function doThing(uint256 thing) internal pure returns (uint256) {
        // return thing / 2;
        return thing.div(2);
    }
}

The call to doThing allocates 64 bytes per call, because of the div call. Replacing thing.div(2) with thing / 2 reduces memory allocation to 0 as expected.

@abcoathup
Copy link
Contributor

Hi @nlordell ! Thanks for the suggestion, it is really appreciated.

The project owner will review your suggestion as soon as they can.

Please wait until we have discussed this idea before writing any code or submitting a Pull Request, so we can go through the design beforehand. We don’t want you to waste your time!

@ashwinYardi
Copy link
Contributor

The same scenario can be reproduced for SafeMath.sub(uint256, uint256) . I confirmed on remix. So, both div and sub allocated 64 bytes per call. Here is the contract used for verifying:

// SPDX-License-Identifier: CC0-1.0
pragma solidity ^0.6.0;

import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.2.0/contracts/math/SafeMath.sol";

contract SafeMathSubMemoryTest {
    using SafeMath for uint256;
    
    function doManyThings(uint256[] calldata things) external pure returns (uint256[] memory result, uint256 mem) {
        result = new uint256[](things.length);
        
        assembly {
            mem := mload(0x40)
        }
        for (uint256 i = 0; i < things.length; i++) {
            result[i] = doThing(things[i]);
        }
        assembly {
            mem := sub(mload(0x40), mem)
        }
    }
    
    function doThing(uint256 thing) internal pure returns (uint256) {
        // return thing - 2;
        return thing.sub(2);
    }
}

@frangio
Copy link
Contributor

frangio commented Dec 2, 2020

Thanks everyone for reporting and testing this. I am very interested in these results and will be looking into it.

Amxx added a commit to Amxx/openzeppelin-contracts that referenced this issue Jan 8, 2021
FIxes OpenZeppelin#2413, remove the string allocation in SafeMath  default functions: caused unrecovered memory allocation even for successfull calls
@Amxx
Copy link
Collaborator

Amxx commented Jan 8, 2021

Note; mod is also affected. This is due to the "default" SafeMath function allocating strings for revert messages, even when they are not actually used.

Fix requiers some code duplication, to make sure that these defaults function do not allocate string unless they are just about to revert. PR #2462 fixes that issue, as shown by the tests. I also used the occasion to add "custom revert message" versions of add and mul

@Amxx Amxx added bug contracts Smart contract code. labels Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug contracts Smart contract code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants