Go Back:

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.

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

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 :

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.

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.

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.

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.

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

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

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)

Self explanatory.

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

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.

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.

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

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.

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:

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:

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

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.

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:

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

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:

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:

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

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

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.

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

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

Example:

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

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.