Go Back:

Gas Optimizations

[GO-01] Post Struct Gas Optimizations

• Line Numbers: 14-23

The title, which can likely be limited to 32 characters can be bytes32 instead of string. String types are variable length and take up more space than a fixed byte array.

Additionally, the struct should be packed in an efficient way. Solidity storage slots are 256 bits, so variables under that limit should be ordered in a manner that minimizes used storage slots.

Next, the deadline variable, which is currently stored as a string, should be stored as a uint256. Not only is this common practice for timestamps, it also makes them easier to work with and more gas efficient.

Next, goal, amount_raised, and num_donors can all be changed to smaller uint type to save gas. Is is unlilkely that any of these variables reach any order of magnitude near $2^{255} - 1$.

Finally all variables but post_id, amount_raised, num_donors should be marked as constant because they will not change once initialized. For updating structs the protocol opts to create a whole new struct and replace the old one.

[GO-02] Profile Struct Gas Optimizations

• Line Numbers: 7, 8, 9, 10

The username variable can be switched from a string type to a bytes32 type. String types are variable length, and as such can take up extra storage space when they are longer, by limiting usernames to be only 32 characters, gas can be saved by preventing extra storage space from being taken up.

The username, profile_pic and bio variables should remain ordered as such, solidity storage slots are sized at 32 bytes. The address field takes up 20 bytes, and as such the first storage slot for the profile struct. The changed bytes32 username variable will take up the next slot, and then the final two profile_pic and bio variables will take up slots afterwards. If the string variables were put before the fixed size variables, gas inefficiencies could occur making the program use more gas than necessary for storage reads/writes in the case that the string is 37 bytes long causing the a 32-byte username to be pushed to the next storage slot

[GO-03] ++i/i++ Should be unchecked{++i}/ unchecked{i++} when it is not possible for them to overflow, as is the case when used in For and While loops

• Line Numbers: 82,126,159

The code should use an unchecked loop iterator. In the case that the programmer knows there won't be an underflow or overflow (i.e a situation where increments will not overflow due to a for loop conditional) the programmer can use an unchecked loop iteration block to skip the automatic under/over flow checks solidity implements on arithmetic operations. See example below :

for (i = 0; i < length;) {
	...//some functionality
	unchecked{
		++i;
	} 
}

[GO-04] Use .call instead of .send or .transfer rather than

• Line Numbers: 166

As of the Istanbul hard fork, it is suggested to replace use of .send(amount) with .call{value: amount}() due to the fixed gas cost assigned while using the former. In other words, using .call saves gas.

[GO-05] Initial bounds check for post_id would save gas

• Line Numbers: 151

If post_id is larger than the number of posts or less than zero, this should be checked before declaring necessary variables for the loop. In an invalid case, the function should halt to avoid spending more gas.

[GO-06] <array>.length should not be looked up in every loop of a For loop

• Line Numbers 82,126,159

The length of arrays should not be accessed every time throughout the loop. Rather, it should be saved in a variable prior to the loop and that variable used to check in the for loop.

uint len = array.length;
for (i = 0; i < len;)
.  .  .  

[GO-07] ++i Costs less gas than i++, especially when it’s used in For loops (--i/i-- Too)

• Line Numbers: 82,126,128,159

i++ can be changed to ++i. This is slightly more gas efficient because it avoids creating a temporary variable. The pre-increment operation is cheaper in the EVM.

[GO-08] It costs more gas to initialize variables to zero than to let the default of zero be applied

• Line Numbers: 81,82,159

In solidity, the default value of uint is zero, so assigning it to zero does nothing and wastes gas.

uint256 x;
// x is set to 0 automatically

[GO-09] Loop with single update condition should break

• Line Numbers: 159-163

In fundPost, only one record is accessed, but the loop continues after this. Consider adding a break inside the if statement.

[GO-10] Public functions not called by the contract should be declared external instead

• Line Numbers: 53,59,70,77,94,115,123,151

If a public function is not called internally (by the contract), it can be marked as external to save gas. (all functions except line 144)

function test(address x) public external {
	. . .
}

[GO-10] x += y costs more gas than x = x + y for state variables

• Line Numbers: 167

Self explanatory.

[GO-11] Struct variables should be marked constant

• Line Numbers: 6-11,14-23

These variables (except post_id, num_donors, and amount_raised) are not changed, so they should be marked constant.

[GO-12] Post array could be placed inside profile

• Line Numbers: 6-11, 29

Because each user address is already associated to a user profile, there is no need to have a separate mapping of users to an array of Posts. Instead, the post array could be placed inside the profile struct, saving gas.

[GO-13] Post array could be mapping

• Line Numbers: 13

Instead of having to iterate through the post array of a user to find a specific post by id, a mapping of post id’s to posts could be used instead to make access times $O(1)$. This would make the overall structure a mapping of addresses to a mapping of post id’s to posts.

[GO-14] createPost function struct could be initialized in one line

• Line Numbers: 101-109

Instead of individually initializing each parameter of the struct, it can be initialized in a single line like so: fooStruct myStruct = fooStruct(1,2);

[GO-15] num_donors and amount_raised shouldn’t be function parameters

• Line Numbers: 106, 108

Upon post initialization, the num_donors and amount_raised should always be initialized to $0$. Therefore there is no need to pass them as function parameters.

[GO-16] Refactor modifier to call local function

• Line Numbers: 34

The validAddress function currently uses require() directly in the modifier, which means that every time the modifier is called, this statement executes. This is expensive and not efficient in terms of gas costs, as it causes unnecessary increases in bytecode size. Consider calling the require() function only once by using an internal function to check if the address is valid.

See example below:

modifier validAddress(address _addr) {
require(isValidAddress(_addr), "Invalid address");
_;
}
function isValidAddress(address _addr) internal view returns (bool) {
    return _addr != address(0);
}

[GO-17] Create custom error

• Line Numbers: 34

Another different way you can clean up the validAddress is by removing the string error messages. While these messages work, you can create custom errors that reduce gas costs compared to the string error messages. How do we do this? Here’s an example of what this could look like:

error InvalidAdress(address addr);
function _isValidAddress(address addr) internal view returns (bool) {
return (addr != address(0));
}
modifier validAddress(address _addr) {
require(_isValidAddress(_addr), InvalidAddress(_addr));
_;
}

Non-Critical Issues

[NC-01] Function parameter should be unsigned to match convention

• Line Numbers: 151

Change signed integer function parameter post_id to unsigned to match it’s declaration in the Post struct.

[NC-02] Loop lookup may not be necessary with provided post_id

• Line Numbers: 159-163

In other functions, post_id is enforced to be the same as the index of the post in the posts array. Using a loop here may not be required. However, though this post_id and index alignment is present in the code, we do recommend against this method, as it is error prone.

Low Risk Issues

[LOW-01] Loop variable should be unsigned, underflow/overflow possible

• Line Numbers: 154

Unsigned integers in Solidity use 256 bits to store a value, which can only be positive. Signed integers have the same number of bits but also store negative values, so the most significant bit is the sign bit (it adds $-2^{256}$ to the number). Thus, the range of signed integers is smaller than the range for unsigned integers. Since the upper bound of the loop is an unsigned integer, overflow could happen at large upper bounds.

Example of issue:

uint256 upper_bound = shiftLeft(1, 255) + 1
for (int i = 0; i < upper_bound; ++i) { 
	// loop iterator overflow before reaching upper bound
}

Fix: Use the same numerical data type for loop iterator and upper bound.

[LOW-02] getPosts returns memory, not storage

• Line Number: 144, 123

The getPosts function currently returns a memory object. This is problematic because the removePost function relies on getPosts to fetch the list of current posts and edit it. The issue is that memory variables are copies of state variables, and do not edit the values themselves. So currently, removePost merely edits a copy of the posts_listarray and does not actually remove posts. Remove the getPosts function entirely and rewrite removePost like so:

function removePost(uint post_idx, address reciever_address) public {
        Post[] storage these_posts = posts_list;
 . . . . 

Medium Risk

[MED-01] Re-entrance risk with message call before state change

• Line Numbers: 166

In the fundPost() function, state changes are made after a .send(), which could lead to a re-entrance attack. In the following example, .call occurs before the state change. Note that this contract should use .call instead of .send as well (see [GO-04]). This issue is not currently exploitable in the current state, but should be fixed immediately.

Example:

uint256 value = 123;
function doSomething(address addr) {
	addr.call{amount: 100}(); // external call
	value += 1; // state change
}

Fix: reverse the two lines for external call and state change.

[MED-02] Storage v Memory

• Line Number: 124

The these_posts variable should be marked with storage instead of memory. This allows for the changes to the variable be persisted to the contracts storage. Using memory would create changes that are thrown out once the function call is complete, effectively making the removePost function not function at all in the program

[MED-03] post_id variable makes potentially problematic assumptions

• Line Number: 128

The post_id is the variable in a Post struct used to identify its index in the post list for an individual address. This implementation assumes that the post_id values are sequential and without gaps or error. This can cause issues in the case that the post_id value is incorrectly assigned, or manipulated in other parts of the program, which could cause the deletion of a post not intended to be deleted. Consider changing the method in which the data is stored in the program to instead employ a mapping of id to post instead of an array of posts, or to use additional identifiers to ensure that the correct post is being deleted.

High Risk

[HIGH-01] Function parameter amount should be unsigned

• Line Numbers: 151

Function parameter amount could be negative and is unchecked. At the current state, .send will most likely due to a negative ether argument, which is not allowed. However, this issue could easily subtract balance from the amount_raised

uint256 amount_raised = 1111;
function test(int param) {
	// consider if param's value is negative
	amount_raised += param; 
}

[HIGH-02] Functions should verify receiver is post/profile creator before allowing update/delete

• Line Numbers: 70,77,123

Currently, any user can create/update/delete any post or profile without authentication.

Example:

function updateProfile(...) {
	// lacking check if caller is profile owner
	delete profile_map[_user];
  Profile memory profile = Profile(_user, _username, _profile_pic, _bio);
  profile_map[_user] = profile;
}

Fix: add a check if the function caller is the profile/post creator.

[HIGH-03] Security Risk

• Line Numbers: 115

The updatePost function has no check to ensure that the user who is updating the post is the original author, so any user can modify any post in the contract. This can lead to unauthorized changes and tampering with the smart contract’s data, making it a serious security risk. While you could add a modifier to check whether the user is the original author, this would add bureaucracy and complexity and increase gas costs. It is recommended to remove updatePost because doing so would avoid both potential security vulnerabilities and unnecessary costs.