Audit "Trustless Rewards"
I was asked to audit the smart contract “Trustless Rewards” in Clarity programming language. The contract is used on the site https://stacksdegens.com/game-lobby.
The purpose of the smart contract is to manage a list of game lobbies with leaderboards. An administratior should be able to set scores per lobby and distribute rewards to lobby guest according to the published scores.
This audit was executed and written by Friedger (OpenIntents). The final report was published on October 17th, 2022.
The contract was already deployed on Stacks mainnet blockchain at the address
'SP1SCEXE6PMGPAC6B4N5P2MDKX8V4GF9QDE1FNNGJ trustless-rewards. The source code is also available on github copied from the blockchain for better review: Trustless Rewards ✎
A git repo was provided that contains tests for the smart contract. The tests for the contract at hosted at https://github.com/BowTiedDeployer/trustless-rewards. For the deployed contract, the git commit eda81d contains the relevant source code. The contract in this repo only differs in some comments and the used traits for SIP9 and SIP10. The deployed contract uses correctly the trait definitions mentioned in the SIP documents.
The audit focussed on the following topics:
- misuse of funds
- error handling
- runtime errors
For the audit, I used
- manual code inspection
- manual tests with clarity console
- reproducable tests
The contract code is well structured and good to read. No issues of high risks were found. Tests cover 100% of the code.
The distribution of rewards is under the full control of the contract administrator. Users can verify how the administrator distributed the rewards because all information is on-chain. However, they have to trust the administrator to execute the distribution as intended.
I have implemented some of the recommendations https://github.com/friedger/trustless-rewards/commit/134de04c4ccdd8a40a963fcee6aa413aa1754588.
More changes have been added to the repository to address the issues mentioned in a first draft shared with the team. The state of the issues is reflected in the summary.
- Security: Use of traits with as-contract (won’t fix)
- Correctness: Mismatch between assets and state (won’t fix)
- Error Handling: No error on wrong id (fixed)
- Error Handling: Use of unwrap-panic (fixed)
- Error Handling: Use of
- Performance: Unnecessary unwrap of
- Performance: Unnecessary unwrap of
map-get?and wrap as result (fixed)
- Performance: Unnecessary unwrap of function call and wrap as result (fixed)
- Performance: Use of tuple with single value (fixed)
- Code Readability: Mix of state data with storage data (won’t fix)
- Code Readability: Duplication of code (fixed)
ft-transfer traits are called in the contract context (
as-contract) potentially giving the traits access to all assets of the contract. For more details, see https://app.sigle.io/friedger.id/HuOT9tNQC8fTXOsK28D7e.
The security risk is limited because these functions can be called only by the administrator (
Recommendations: Call these functions only with whitelisted contracts and always use appropriate postconditions.
stx-transfer allows the administrator (
contract-owner) to change the assets owned by the contract while there is no on-chain information about the available funds. The function is probably intended to transfer commission fees. However, this functions can be abused by the administrator.
finish-result-many allows the administrator (
contract-owner) to transfer any amount of Stacks tokens out of the contract.
The risk is limited because the function can be called only by the administrator (
Recommendations: Add a
total-current-balance variable holding the current balance of all lobbies and guard the
stx-transfer function (and even
ft-transfer for the reasons mentioned above) with a check whether there is a mismatch between the stx balance of the contract and the current total balance. Similarily, add a check in the
finish function so that no more than the current balance of the lobby can be transferred out. The current balance will always be below or equal to the
balance (total funds raised).
disable-lobby, the result is the same whether the lobby exists or not.
Recommendations: Return with an error if the lobby does not exists in the map.
add-balance, the call to
stx-transfer is wrapped in
unwrap-panic!. Thereby, errors in
stx-transfer are not visible for the caller.
try! instead and return value of type
(Response bool uint). Then wrap calls to
add-balance in a
publish, to check whether a user has join the relevant lobby
unwrap-panic is used
Recommendations: Similar to the checks for active status of the lobby and administration privileges of the transaction caller, the check for lobby membership should use
asserts! together with
add-balance, the result of
map-get? is unwrapped in a
match call and in a
join, the map result is unwrapped three times.
let to define variable
get-score, the result of
map-get? is unwrapped and then wrapped into an
Recommendations: Return just the result of
map-get? for the read-only function.
In private functions
finish-only, the result of
finish is unwrapped and then wrapped into an
Recommendations: Use function result directly as result and remove
lobbies uses a tuple with a single uint value as the key type.
(define-map lobbies uint ...) instead. For code readability, add a comment that the uint represents the id of the lobby.
create-lobby, the map
lobbies stores both data relevant for the state of the contract and data for storage in one tuple.
Recommendations: In map
lobbies use tuple value containing one tuple
meta-data for storage data and one tuple
values for state relevant data. The tuple
values should contain value
finish only differ in a function call
Recommendations: In function
finish call the