This document is a security audit report performed by RideSolo, where TheWALL has been reviewed.
- thewall.sol github commit hash 163e39b888c2364c708139d0916db6e63c4acc59.
- thewallbeneficiaries.sol github commit hash 163e39b888c2364c708139d0916db6e63c4acc59.
- thewallcore.sol github commit hash 163e39b888c2364c708139d0916db6e63c4acc59.
- thewallcoupons.sol github commit hash 163e39b888c2364c708139d0916db6e63c4acc59.
- thewallusers.sol github commit hash 163e39b888c2364c708139d0916db6e63c4acc59.
13 issues were reported including:
- 3 high severity issue.
- 2 medium severity issue.
- 4 low severity issue.
- 4 Owner privileges.
The number of area to be added in a cluster should be limited either when using createMulti or when adding area to cluster one by one using _addToCluster or _create since function such as _removeCluster or _removeFromCluster can break due to a for loop that iterate on the cluster area array.
Other issues can raise like if an owner want to transfer an area that is inside a cluster, _canBeTransferred will throw since it doesn't allow area transfer that is within a cluster, if the previously described issue happen it will lock the users area from being transfred or sold to anoter user since he won't be able to remove it from his cluster.
if a newly created area using _create function member of TheWallCore do not meet the following condition, if (_abs(x) <= 100 && _abs(y) <= 100) a random value is computed through _rand function implemented in the same contract to give the premium value to that area or not.
_rand is relying on the blockhash of the last block. An attacker can make an exploit contract with the same PRNG code in order to call the target contract via an internal message. The “random” numbers for the two contracts will be the same. thus guessing if the create block will be premium or not.
the previous hack can be combine with an offchain verification of the newly created block hash to check if the premium value will be met or not then calling the hacking contract to ensure that the trasaction execution is done on that next block only.
whitelisted admins are allowed to make delagate calls to external contract from TheWallCoupons, TheWall, TheWallBeneficiaries and TheWallCore, knowing that delegateCall allow its user to execute code contained in functions of a contract in the context of the calling contract, meaning that a malicious or erroneous code that wasn't audited can be executed on the audited contracts. Multiple risks can be highlighted like:
- Execution of erroneous code that will end by breaking the contracts logic or result of fund loss.
- Admin private key hack.
- Malicious admin.
In conclusion for this issue the audit cannot guarantee the safety of the users since external code can be applied to the actual contract variables without prior audit.
https://github.com/RideSolo/TheWall/blob/163e39b888c2364c708139d0916db6e63c4acc59/thewall.sol#L311#L314 https://github.com/RideSolo/TheWall/blob/163e39b888c2364c708139d0916db6e63c4acc59/thewallbeneficiaries.sol#L131#L134 https://github.com/RideSolo/TheWall/blob/163e39b888c2364c708139d0916db6e63c4acc59/thewallcore.sol#L532#L535 https://github.com/RideSolo/TheWall/blob/163e39b888c2364c708139d0916db6e63c4acc59/thewallcoupons.sol#L97#100
_create function member of TheWallCore can be called by admins, however when assigning the tokenId
_areasOnTheWall[x][y] no verification is done to check if a tokenId was preassigned to the same area.
same issue is also applicable to tokenId itself since the non-fungible erc721 token is create by the same function, _tokens[tokenId] meaning that if a tokenId was previously created it can be overwritten by an admin by mistake or intentionaly.
create function member of TheWall validate if the area is empty by calling _core._canBeCreated(x, y), meaning that public calls are checked for area overwritting but the restricted function is not, admin or public user can do the same mistakes and should be checked the same way.
Please note that the compiler raised the following issue "Warning: Return value of low-level calls not used. a.delegatecall(b)", delegateCall return two variable (bool, bytes) that must be handled.
https://github.com/RideSolo/TheWall/blob/163e39b888c2364c708139d0916db6e63c4acc59/thewall.sol#L311#L314 https://github.com/RideSolo/TheWall/blob/163e39b888c2364c708139d0916db6e63c4acc59/thewallbeneficiaries.sol#L131#L134 https://github.com/RideSolo/TheWall/blob/163e39b888c2364c708139d0916db6e63c4acc59/thewallcore.sol#L532#L535
The implemented _burn function is not part of ERC223 standard and should be removed since it allows the admins to burn users tokens without permission refer to ERC223 description to implement the burning logic correctly.
_useCoupons function member of TheWallUsers uses _burn function to burn coupons once used however a different logic can be implemented to avoid such drastic solution like simply transfering the coupons to the contract and using ERC223 tokenFallback since it was intended to handle transfer event on the contract side.
- Check coupons contract address to be different than zero inside the constructor of
TheWallUsers. - Check the updated coupons contract address to be different than zero here;
- Admin can change the coupons contract address at any moment here.
Most function implemented on TheWallCore can be called by an admin that is different than TheWall contract address, since it is deployed independently from the main contract, multiply privileges issues can raise like the following:
- Admin can set any token for sale instead of the owner calling
_forSalemember ofTheWallCore. - Admin can set any token for rent instead of the owner by calling
_forRentmember ofTheWallCore. - Admin can remove cluster using
_removeClusterwithout owner permission. - Admin can buy, rent, rentTo, cancel, finish rent,
_addToCluster,_removeFromClusterwithout owner permission since all input variables on those functions can be directly manipulated by the admin in the opposit of the non restricted function implemented inTheWallcontract.
createMulti member of TheWall contract should be limited to a fix number of area to be created since it can reach transaction gas limit or block gas limit.
When setting the wall size using setWallSize member of TheWallCore, the new size should not exclude any area that was created in the wall extremity, meaning that if the wall size is shrinked then some area can be excluded from it. there is no direct implication following my understanding however the UI must follow some rule and it might collide with this issue.
The audited contract are not safe, developers should resolve all issues before live deployement.