Merge pull request #23 from blocksecteam/nft-owner-check

Detector: nft-owner-check

Makefile

@@ -284,6 +284,16 @@ nft-approval-check: tg_ir
 	fi
 	@cat ${TMP_DIR}/.bitcodes.tmp | xargs -i $(LLVM_OPT) ${OPTFLAGS} -load detectors/$(subst -,_,$@).so -$@ {} -o /dev/null
 
+nft-owner-check: tg_ir
+	@rm -f ${TMP_DIR}/.$@.tmp
+	@make -C detectors $(subst -,_,$@).so
+	@if test $(shell cat ${TMP_DIR}/.bitcodes.tmp | wc -c) -gt 0 ; then \
+		figlet $@ -w 200 ; \
+	else \
+		echo -e "\e[31m[!] Source not found\e[0m" ;  #]] \
+	fi
+	@cat ${TMP_DIR}/.bitcodes.tmp | xargs -i $(LLVM_OPT) ${OPTFLAGS} -load detectors/$(subst -,_,$@).so -$@ {} -o /dev/null
+
 tautology:
 	@rm -f ${TMP_DIR}/.$@.tmp
 	@if test $(shell find ${NEAR_SRC_DIR}// -name '*.rs' | wc -c) -gt 0 ; then \

README.md

@@ -100,6 +100,7 @@ All vulnerabilities **Rustle** can find.
 | `incorrect-json-type`   | [incorrect type used in parameters or return values](docs/detectors/incorrect-json-type.md) | High     |
 | `unsaved-changes`       | [changes to collections are not saved](docs/detectors/unsaved-changes.md)                   | High     |
 | `nft-approval-check`    | [find `nft_transfer` without check of `approval id`](docs/detectors/nft-approval-check.md)  | High     |
+| `nft-owner-check`       | [find approve or revoke functions without owner check](docs/detectors/nft-owner-check.md)   | High     |
 | `div-before-mul`        | [precision loss due to incorrect operation order](docs/detectors/div-before-mul.md)         | Medium   |
 | `round`                 | [rounding without specifying ceil or floor](docs/detectors/round.md)                        | Medium   |
 | `lock-callback`         | [panic in callback function may lock contract](docs/detectors/lock-callback.md)             | Medium   |

detectors/nft_owner_check.cpp

@@ -0,0 +1,78 @@
+/**
+ * @file nft_owner_check.cpp
+ * @brief check if there is an owner check in priviledged function
+ *
+ */
+#include "near_core.h"
+
+#include <fstream>
+#include <set>
+
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/DebugLoc.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LegacyPassManager.h"
+#include "llvm/Pass.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Regex.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/IPO/PassManagerBuilder.h"
+
+namespace {
+    struct NftOwnerCheck : public llvm::ModulePass {
+        static char ID;
+
+      private:
+        llvm::raw_fd_ostream *os = nullptr;
+
+        std::set<std::string> callbacks;
+
+        llvm::Regex const regex_nft_approve_function = llvm::Regex("[0-9]+(nft_approve|nft_revoke|nft_revoke_all)[0-9]+");  // `nft_revoke_all` is not priviledged
+
+      public:
+        NftOwnerCheck() : ModulePass(ID) {
+            std::error_code EC;
+
+            os = new llvm::raw_fd_ostream(std::string(getenv("TMP_DIR")) + std::string("/.nft-owner-check.tmp"), EC, llvm::sys::fs::OpenFlags::OF_Append);
+
+            std::ifstream is;
+            is.open(Rustle::callback_file);
+            std::string callbackLine;
+            while (is >> callbackLine) {
+                callbacks.insert(callbackLine.substr(0, callbackLine.find('@')));
+            }
+            is.close();
+        }
+        ~NftOwnerCheck() { os->close(); }
+
+        bool runOnModule(llvm::Module &M) override {
+            using namespace llvm;
+
+            for (auto &F : M.functions()) {
+                if (!Rustle::debug_check_all_func && Rustle::regexForLibFunc.match(F.getName()))
+                    continue;
+                if (F.getName().contains("$closure$") || !regex_nft_approve_function.match(F.getName()))  // only check selected functions
+                    continue;
+
+                if (Rustle::debug_print_function)
+                    Rustle::Logger().Debug("Checking function ", F.getName());
+
+                *os << F.getName();
+                if (Rustle::isFuncPrivileged(&F)) {
+                    Rustle::Logger().Info("Find owner check for \e[34m ", F.getName());
+                    *os << "@True\n";
+                } else {
+                    Rustle::Logger().Warning("Lack owner check for \e[34m", F.getName());
+                    *os << "@False\n";
+                }
+            }
+            return false;
+        }
+    };
+}  // namespace
+
+char NftOwnerCheck::ID = 0;
+static llvm::RegisterPass<NftOwnerCheck> X("nft-owner-check", "", false /* Only looks at CFG */, false /* Analysis Pass */);
+
+static llvm::RegisterStandardPasses Y(llvm::PassManagerBuilder::EP_EarlyAsPossible, [](const llvm::PassManagerBuilder &builder, llvm::legacy::PassManagerBase &PM) { PM.add(new NftOwnerCheck()); });

docs/detectors/nft-owner-check.md

@@ -0,0 +1,22 @@
+## Lack of check approval id during NFT transfer
+
+### Configuration
+
+* detector id: `nft-owner-check`
+* severity: high
+
+### Description
+
+In the design of the [NEP178](https://github.com/near/NEPs/blob/master/neps/nep-0178.md), the owner of the NFT can approve or revoke approvals by using the specified interfaces (ie, `nft_approve`, `nft_revoke` and `nft_revoke_all`). An owner check should be implemented for these interfaces to make sure they are callable to the owner only, otherwise, anyone can modify the approvals of the NFT. 
+
+### Sample code
+
+Code in [near-contract-standards](https://github.com/near/near-sdk-rs/blob/a903f8c44a7be363d960838d92afdb22d1ce8b87/near-contract-standards/src/non_fungible_token/approval/approval_impl.rs) shows the correct implementation.
+
+```rust
+// should be implemented for `nft_approve`, `nft_revoke` and `nft_revoke_all`
+let owner_id = expect_token_found(self.owner_by_id.get(&token_id));
+let predecessor_account_id = env::predecessor_account_id();
+
+require!(predecessor_account_id == owner_id, "Predecessor must be token owner.");
+```